Components: Refactor Placeholder tests to @testing-library/react #43069
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
walbo
added
[Type] Enhancement
|
Size Change: -6 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
mirka
reviewed
Aug 10, 2022
Comment on lines
56
to
59
| // Test for empty label. | ||
| const label = getLabel(); | ||
| expect( label ).toBeInTheDocument(); | ||
| expect( label ).toBeEmptyDOMElement(); |
There was a problem hiding this comment.
I just noticed that label is marked as required in the readme, and judging from the visual design I think that is correct. Should we fix the TS type to be required, rather than test for the empty case? I don't think it should happen in proper usage anyway.
There was a problem hiding this comment.
Did a search to check how Placeholder is used inside Gutenberg, and there are a few places it is used without a label. Ex:
gutenberg/packages/block-library/src/block/edit.js
Lines 105 to 107 in c3f19c4
gutenberg/packages/block-library/src/cover/edit/index.js
Lines 359 to 362 in c3f19c4
Seems that it was me in #42990 that didn't doublecheck that the readme and types matched. I feel that it makes sense to mark it as required, but not sure since it are used multiple places without one.
There was a problem hiding this comment.
Wow. I'm not familiar enough with this component to make a decision, so let's leave it be then 😅 We can revisit when necessary.
There was a problem hiding this comment.
Yep. Sound good to me. Changed the required to No in this PR to avoid confusion.
Comment on lines
35
to
38
| const getLabel = () => { | ||
| const placeholder = getPlaceholder(); | ||
| return placeholder.querySelector( '.components-placeholder__label' ); | ||
| }; |
There was a problem hiding this comment.
I don't love this... I feel like the label should have some kind of meaningful semantics and not be a stray piece of text 🤔 But that's more of a problem with the component itself, so we'll have to leave it be for the purpose of this PR!
There was a problem hiding this comment.
Should we add a comment about it, at least?
There was a problem hiding this comment.
👍
Updated the test to only use querySelector when we check if the elements is empty/don't exists. Added a comment on the where we use querySelector.
mirka
approved these changes
Aug 15, 2022
There was a problem hiding this comment.
Looking good, thank you! 🚀
I guess a trend we're seeing in these legacy tests is a high amount of CSS class name assertions (also #42981). RTL purity aside, a lot of them are murky in their purpose. Are they intended to test logic, for example with complicated style variants? Are they intended to test styles, however unreliable that is? Are they intended to be a contract to the consumer that certain elements will have certain classes?
It isn't necessarily something we have to tackle in these RTL refactors, but it's something we'll have to clean up as part of #43083 anyway. I just wanted to mention that as long-term context as we continue to work on these refactors 🙂
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?
We've recently started refactoring
enzymetests to@testing-library/react.This PR refactors the
<Placeholder />component tests fromenzymeto@testing-library/react.Why?
@testing-library/reactprovides a better way to write tests for accessible components that is closer to the way the user experiences them.Refactoring the test will also make it posible to add typescript to them. Related PR #42990
How?
Replace
enzymetests with@testing-library/reactones.Testing Instructions
Verify tests pass:
npm run test:unit packages/components/src/placeholderScreenshots or screencast