ESLint: Fix jest/expect-expect violations
#47219
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
tyxla
added
[Type] Automated Testing
tyxla
requested review from
Mamaduka,
mirka,
jsnajdr,
ciampo,
flootr,
chad1008,
ntsekouras and
brookewp
tyxla
commented
Jan 17, 2023
| @@ -1,3 +1,5 @@ | |||
| /* eslint jest/expect-expect: ["warn", { "assertFunctionNames": ["expect", "expectTokensToBeInTheDocument", "expectTokensNotToBeInTheDocument", "expectVisibleSuggestionsToBe", "expectEscapedProperly"] }] */ | |||
There was a problem hiding this comment.
We're intentionally extending the list of assert functions since we're using some custom ones in this suite.
| @@ -83,6 +85,17 @@ const expectTokensNotToBeInTheDocument = ( tokensText: string[] ) => { | |||
| ); | |||
| }; | |||
|
|
|||
| const expectEscapedProperly = ( tokenHtml: string ) => { | |||
There was a problem hiding this comment.
This is being extracted since it's used in 2 tests below.
We're also naming it expect* to match the assertion function name convention.
Comment on lines
+1249
to
+1271
| it( 'should be still used to filter out duplicate suggestions', async () => { | ||
| const user = userEvent.setup(); | ||
|
|
||
| render( | ||
| <FormTokenFieldWithState | ||
| __experimentalExpandOnFocus | ||
| initialValue={ [ { value: 'France' }, { value: 'Spain' } ] } | ||
| initialValue={ [ | ||
| { value: 'Slovenia' }, | ||
| { value: 'Spain' }, | ||
| ] } | ||
| suggestions={ [ 'Slovenia', 'Slovakia', 'Sweden' ] } | ||
| /> | ||
| ); | ||
|
|
||
| const input = screen.getByRole( 'combobox' ); | ||
|
|
||
| // Typing `slov` will match both `Slovenia` and `Slovakia`. | ||
| await user.type( input, 'slov' ); | ||
|
|
||
| // However, `Slovenia` is already selected. | ||
| expectVisibleSuggestionsToBe( screen.getByRole( 'listbox' ), [ | ||
| 'Slovakia', | ||
| ] ); |
There was a problem hiding this comment.
I'm actually writing this test up since previously it was only rendering and testing nothing in particular.
cc @ciampo for feedback, since he recently worked on refactoring the tests.
Comment on lines
-1519
to
-1527
| ].forEach( ( tokenHtml ) => { | ||
| screen.getByText( ( _, node: Element | null ) => { | ||
| if ( node === null ) { | ||
| return false; | ||
| } | ||
|
|
||
| return node.innerHTML === tokenHtml; | ||
| } ); | ||
| } ); |
There was a problem hiding this comment.
Being extracted to a separate function above, as mentioned above.
Comment on lines
-1549
to
-1557
| ].forEach( ( tokenHtml ) => { | ||
| screen.getByText( ( _, node: Element | null ) => { | ||
| if ( node === null ) { | ||
| return false; | ||
| } | ||
|
|
||
| return node.innerHTML === tokenHtml; | ||
| } ); | ||
| } ); |
There was a problem hiding this comment.
Being extracted to a separate function above, as mentioned above.
| @@ -10,6 +10,7 @@ import { contextConnect, contextConnectWithoutRef } from '../context-connect'; | |||
| import type { WordPressComponentProps } from '../wordpress-component'; | |||
|
|
|||
| // Static TypeScript tests | |||
| /* eslint-disable jest/expect-expect */ | |||
There was a problem hiding this comment.
This contains static typescript tests, so intends to use TS for checking for failures, and not Jest. While I have mixed feelings about this manifestation of tests, this intentionally doesn't contain assertion functions, so we're intentionally disabling the rule here.
| @@ -14,6 +14,7 @@ import { forwardRef } from '@wordpress/element'; | |||
| import type { WordPressComponentProps } from '../wordpress-component'; | |||
|
|
|||
| // Static TypeScript checks | |||
| /* eslint-disable jest/expect-expect */ | |||
There was a problem hiding this comment.
This contains static typescript tests, so intends to use TS for checking for failures, and not Jest. While I have mixed feelings about this manifestation of tests, this intentionally doesn't contain assertion functions, so we're intentionally disabling the rule here.
| @@ -32,7 +32,9 @@ describe( 'CustomTemplatedPathPlugin', () => { | |||
| } ); | |||
|
|
|||
| it( 'should resolve with basename output', async () => { | |||
| await webpackAsync( config ); | |||
| await accessAsync( outputFile ); | |||
| expect( async () => { | |||
There was a problem hiding this comment.
This actually meant to verify that it's not throwing in the meantime, so we're adding that assertion to make it a full-featured test.
| 'Each block has its own settings. To find them, tap on a block. Its settings will appear on the toolbar at the bottom of the screen.'; | ||
| await waitForElementToBeRemoved( () => screen.getByText( text ) ); | ||
|
|
||
| expect( screen.queryByText( text ) ).toBeNull(); |
There was a problem hiding this comment.
I'm adding the assertion here to make it a full-featured test. We could use .not.toBeInTheDocument() but that's not supported by our custom React Native implementation yet, thus the toBeNull() usage.
| 'Problem(s) with fixture files:\n\n' + errors.join( '\n' ) | ||
| ); | ||
| } | ||
| } ).not.toThrow(); |
There was a problem hiding this comment.
All we're doing here is making sure this doesn't throw. That's what the test intended, since it throws itself.
It looks like a big changeset, but most of it is just whitespace.
|
Size Change: 0 B Total Size: 1.33 MB ℹ️ View Unchanged
|
|
Flaky tests detected in 3f5c2f2. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/3940062906
|
Mamaduka
approved these changes
Jan 23, 2023
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.
What?
This PR fixes all of the
jest/expect-expectviolations that should not exist.Why?
Ideally, the project should not have any ESLint errors and warnings. We already removed all errors, and this PR cuts many of the existing warnings.
How?
We're extending the rule when needed. We're also finishing up a few tests that were not quite finished. We're ignoring the rule where that's the intended way to move forward.
We're leaving three violations of the rule intact - those are expected, since these tests are empty and still have to be written. The rest of the warnings are violating the
jest/no-disabled-tests, so this PR isn't touching them intentionally.Testing Instructions
npm run lint:jsand verify that the onlyjest/expect-expectviolations are the ones in the screenshot below.Testing Instructions for Keyboard
None.
Screenshots or screencast