Complain if expect is passed multiple arguments #4237
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
When using unexpected it's common to import it under the name
expect
. Consider this case:If a user by accident forgets to import unexpected, they will not immediately notice, as the builtin expect in jest will ignore any further arguments passed to it. Which will result in a passing test instead of the failure you would expect.
This PR adds a check to the expect method, so that it will throw an error if it is called with more than one argument.
@Munter already discussed a solution to this with @cpojer. The solution I implemented here is a bit different. Instead of using a check on
arguments.length
I opted to use...rest
and check that it has a length of 0. The expect method was implemented as an arrow function, and arrow functions do not exposearguments
, so it was either refactoring it into a normal function or using the...rest
alternative. From my humble benchmarking I didn't observe any significant difference, so I opted for the least invasive change, that also happens to be following the Code Conventions the closest - prefering es6 syntax where possible.To further enhance the check one could add a check for the case of expect being called with no arguments. It's less likely to cause users to shoot themselves in the feet, so I left it out.
Test plan
The test implemented covers the case that the check is implementing.
All tests in the project is passing. I ran
yarn test
in the root of the repo to verify that the change had no unintended side effects.