Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More automated tests #152

Merged
merged 6 commits into from
May 2, 2024
Merged

Conversation

puskin94
Copy link
Contributor

Added more automated tests, chipping in for the 100% test coverage goal mentioned in here

I really don't know the project, if not for what I studied while writing tests, so I picked the least complicated route and wrote a couple of automated tests of "well exposed" functions and methods

Copy link
Member

@gurgunday gurgunday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Contributor

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

basename.test.js should cover all branches
busboy-emit.test.js should check if the first call to busboy.emit('finish') returns anything
dicer-write.test.js does not test what it claims

@puskin94
Copy link
Contributor Author

busboy-emit.test.js should check if the first call to busboy.emit('finish') returns anything

@Uzlopak am I reading the code wrong, or Busyboy.emit will never return anything?

})
})

t.end()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically not testing anything, because you call t.end(). use t.plan for this subtest also.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Uzlopak oh, good to know! First time using this testing framework!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove the t.end() please

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Uzlopak is there anything else for me to do in this PR?

@gurgunday gurgunday requested a review from Uzlopak April 19, 2024 19:45
@Uzlopak Uzlopak merged commit e62384c into fastify:master May 2, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants