chore: add eslint-plugin-jest's valid-expect rule #16332
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SimenB
commented
Aug 9, 2019
packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js
Outdated
Show resolved
Hide resolved
SimenB
commented
Aug 9, 2019
SimenB
commented
Aug 9, 2019
|
No significant bundle size changes to report. Generated by 🚫 dangerJS |
SimenB
commented
Aug 9, 2019
| expect(toHaveAttribute(genMarkup({className: 'a'}), ['class', 'a'])); | ||
| expect(toHaveAttribute(genMarkup({className: 'a b'}), ['class', 'a b'])); | ||
| expect(toHaveAttribute(genMarkup({className: ''}), ['class', ''])); | ||
| expect(toHaveAttribute(genMarkup({className: 'a'}), ['class', 'a'])).toBe( |
There was a problem hiding this comment.
not really related to this PR, but toHaveAttribute should probably be written as a custom matcher rather than a function which returns a boolean.
threepointone
approved these changes
Aug 9, 2019
packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js
Outdated
Show resolved
Hide resolved
threepointone
added a commit
to threepointone/react
that referenced
this pull request
Aug 9, 2019
These were discovered by @SimenB in facebook#16332. We weren't making actual assertions on some values. This PR makes the assertions, and fixes the tests.
threepointone
added a commit
to threepointone/react
that referenced
this pull request
Aug 9, 2019
These were discovered by @SimenB in facebook#16332. We weren't making actual assertions on some values. This PR makes the assertions, and fixes the tests.
|
Feel free to rebase from master, the fixes have landed |
threepointone
pushed a commit
that referenced
this pull request
Aug 9, 2019
SimenB
force-pushed
the
eslint-jest-valid-expect
branch
from
ffcf919 to
57125d4
Compare
|
Awesome, thanks for dealing with it! |
|
Oops, this still needs a merge, sorry |
SimenB
force-pushed
the
eslint-jest-valid-expect
branch
from
bd50029 to
962cdf5
Compare
|
rebased |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
There are some assertions here which just doexpect(thing)without a matcher, meaning it's not actually asserting anything (it does not work likeassert). I added.toBe(true)to them, and 2 of the tests are failing, meaning they are either wrong, or false negatives on master.The above is dated now that #16336 was merged, but it did uncover them 🙂