ToolsPanel: refactor unit tests to TypeScript
#48275
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
ciampo
added
[Type] Code Quality
ciampo
requested review from
mirka,
tyxla,
chad1008,
andrewserong and
aaronrobertshaw
|
Size Change: +217 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in e131cba. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4232322647
|
tyxla
approved these changes
Feb 21, 2023
| hasValue: jest.fn().mockImplementation( () => { | ||
| return !! controlProps.attributes.value; |
There was a problem hiding this comment.
This self-referencing was quite awkward. Thanks for fixing those!
There was a problem hiding this comment.
Yeah — I appreciate that it's self contained, but it's a bit akward and prone to breakage (and in fact TS doesn't like it)
There was a problem hiding this comment.
I'm in general not a fan of this "shared state" patten across tests, but we have more pressing priorities at the moment
There was a problem hiding this comment.
Yeah, not a huge deal and something we can easily address if needed.
Comment on lines
+1166
to
+1169
| expect( disabledResetAllItem ).toHaveAttribute( | ||
| 'aria-disabled', | ||
| 'true' | ||
| ); |
There was a problem hiding this comment.
Any reason to not use .toBeDisabled() here?
There was a problem hiding this comment.
Unfortunately toBeDisabled doesn't take the aria-disabled attribute into account (see the docs and testing-library/jest-dom#144)
| } ); | ||
| expect( resetAllItem ).toBeInTheDocument(); | ||
| expect( resetAllItem ).toHaveAttribute( 'aria-disabled', 'false' ); |
There was a problem hiding this comment.
Any reason to not use .not.toBeDisabled() here?
There was a problem hiding this comment.
Same as #48275 (comment)
| @@ -1126,9 +1160,13 @@ describe( 'ToolsPanel', () => { | |||
| expect( announcement ).toHaveAttribute( 'aria-live', 'assertive' ); | |||
|
|
|||
| const disabledResetAllItem = await screen.findByRole( 'menuitem', { | |||
| disabled: true, | |||
There was a problem hiding this comment.
I was curious why we preferred to have a separate assertion for this when we can just have it in the query?
There was a problem hiding this comment.
That's because apparently the byRole selector doesn't accept the disabled option (docs) — which means that effectively this test was not testing what it was supposed to!
There was a problem hiding this comment.
You can thank TypeScript :D
|
Nice one, thanks for refactoring this @ciampo! |
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?
Part of #35744
Refactor the unit tests for the
ToolsPanelcomponent to TypeScript, and therefore marks the end of the component's rewrite to TypeScript.Why?
See #35744
How?
Most errors were solved by adding types, or by refactoring the code in order to better suite TSC's inference.
See each commit for more details on all the changes.
Testing Instructions
No runtime changes — the only file changes is the tests file.