Navigation: add controlled unit test, use modern Testing Library features#41668
Merged
Navigation: add controlled unit test, use modern Testing Library features#41668
Navigation: add controlled unit test, use modern Testing Library features#41668Conversation
ciampo
added
[Type] Automated Testing
|
Size Change: 0 B Total Size: 1.24 MB ℹ️ View Unchanged
|
chad1008
reviewed
Jun 13, 2022
| await user.click( screen.getByRole( 'button', { name: 'Item 2' } ) ); | ||
| expect( | ||
| screen.getByRole( 'button', { name: 'Item 2' } ).parentElement | ||
| ).toHaveClass( 'is-active' ); |
Contributor
There was a problem hiding this comment.
Would it be worthwhile to accompany this with a check that Item 1 no longer has the is-active class? I notice we do something similar up in the first test of the suite, essentially confirming that there's only one active item at a time.
If yes, there may be additional places to apply this to is-active tests elsewhere in the test case as well.
Member
There was a problem hiding this comment.
Along those lines, I wonder if we should instead be checking for aria-current rather than the .is-active CSS. It would also make it easier to assert that no other links/buttons have an aria-current.
expect( screen.getByRole( 'link', { current: 'page' } ).toHaveTextContent( 'Item 2' );...aaaand I was confused a bit why this doesn't work, and apparently our RTL is quite outdated! The current filter landed in @testing-library/dom v8.2.0.
lena@Sieglinde gutenberg % npm ls @testing-library/dom
gutenberg@13.4.0 /Users/lena/Documents/Work/Automattic/gutenberg
`-- @testing-library/react@11.2.2
`-- @testing-library/dom@7.29.0
And unfortunately for us, the DOM Testing Library version bump landed in RTL v13, which drops support for React 17! So uh... never mind I guess 🙃
Side note: I bet we'll be experiencing more of these "wait why isn't this working?" moments, since the RTL docs aren't versioned. Maybe wp-components should have versioned docs at some point, too!
Contributor
Author
There was a problem hiding this comment.
Opened #41710 to try updating RTL-related dependencies to the latest version supporting React 17.
@testing-library/react@12.1.5 seems to pull @testing-library/dom@8.13.0 , so maybe we're still able to add support for current 🤞
Contributor
Author
Member
There was a problem hiding this comment.
Wow that was painless, didn't realize it was backported! Thank you for the quick version bump. Code looks super slick now 😎
mirka
approved these changes
Jun 14, 2022
Member
mirka
left a comment
mirka
left a comment
There was a problem hiding this comment.
Changes look good to me 👍 How do you want to move forward? Chad's point is valid, but we could also wait until we can use the aria-current filter so we can address it better in one go. I don't mind either way.
ciampo
force-pushed
the
feat/navigation-rewrite-logic
branch
from
7df5aed to
88a8fb7
Compare
ciampo
force-pushed
the
feat/navigation-rewrite-logic
branch
from
88a8fb7 to
ad82fac
Compare
ciampo
commented
Jun 14, 2022
| expect( | ||
| screen.getByRole( 'button', { name: 'Item 1' } ).parentElement | ||
| ).toHaveClass( 'is-active' ); | ||
| screen.getByRole( 'link', { current: 'page' } ) |
Contributor
Author
There was a problem hiding this comment.
Note: only "Item 1" and "Item 2" have role="link" since, in the example, their menu items are passed an href attribute.
The rest of the menu items are rendered as button as expected
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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?
In the context of #41612, this PR enhances the unit tests for the
Navigationexperimental componentWhy?
More tests can help in making sure that a refactor doesn't introduce breaking changes.
How?
@testing-library/user-eventinstead offireEventtoHaveTextContent,toBeInTheDocument,toHaveAttribute...)Testing Instructions
Code for the
Navigationunit tests looks good, unit tests pass