Components: Refactor Tooltip tests to @testing-library/react
#43061
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
t-hamano
added
[Type] Enhancement
|
Size Change: +149 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
gziolo
added
the
[Type] Automated Testing
I'd love to, but currently don't have the capacity to work on it systematically. We're trying to chip at it when we can as part of other work on components — but I know that @tyxla has been working on the switch to RTL too! |
|
@ciampo, processing 60 files isn't a simple effort. I feel you 😄 Anyway, if someone wants to champion the coordination process that would be ace! |
|
The existing test passed, but I was looking for any other tests needed. |
t-hamano
commented
Aug 9, 2022
t-hamano
commented
Aug 9, 2022
t-hamano
changed the title
Tooltip tests to @testing-library/reactTooltip tests to @testing-library/react
tyxla
reviewed
Aug 9, 2022
ciampo
reviewed
Aug 10, 2022
There was a problem hiding this comment.
Thank you @t-hamano for your work and @tyxla for your help in reviewing!
I left a few comments, but from my point of view a lot of these tests are very implementation details-focuses. I'd be tempted to determine a new list of what we should be testing for Tooltip based on user behavior and interactions (WDYT, @mirka ?)
Looking at these tests, I'm starting to realize that our current implementation is missing a I wanted to mention this now because I think it means we have less pressure to write a perfectly comprehensive set of tests here, since it's unlikely we'd be able to bulk replace the existing Tooltip usage anyway. (Sidenote: These RTL refactors are quickly exposing semantic gaps in our components, which is great 😄) |
|
I removed |
|
Thanks for the detailed review 😊 I would further suggest the following improvements, does this make sense? Use meaningful button names
Test name standardizationUnify Don't use settimeoutI am not sure if this is the correct way to do it, it looks like it could be rewritten simply as follows: jest.useFakeTimers();
setTimeout( () => {
expect( screen.getByText( 'Help text' ) ).toBeInTheDocument();
jest.runOnlyPendingTimers();
jest.useRealTimers();
}, TOOLTIP_DELAY );
// to...
act( () => jest.advanceTimersByTime( TOOLTIP_DELAY ) );
expect( screen.getByText( 'Help text' ) ).toBeInTheDocument();If the latter implementation is a real timer instead of a fake timer, is there a performance problem? 🤔 |
ciampo
reviewed
Aug 18, 2022
There was a problem hiding this comment.
Hey @t-hamano , thank you for working on this!
I've left more feedback, mostly focused on these changes:
- let's not use
fakeTimersandsetTimeout— we can usejest.advanceTimersByTimeinstead and have tests written in a much more linear fashion - let's always query the
buttons (and the DOM in general) by specifying the expected accessiblename - let's merge similar tests, and remove redundant ones
- let's not pass the same mock function for two separate callbacks
- let's not use the term
popover, as it hints at an internal implementation detail — let's usetooltipinstead
Can you add an entry to the CHANGELOG under the "Internal" section?
Thank you!
…' test without and
…re the tooltip has shown
|
I appreciate your painstaking review. Note that the test command has changed because trunk was merged. |
ciampo
approved these changes
Aug 22, 2022
There was a problem hiding this comment.
Can we add a CHANGELOG entry under the Internal section ?
Apart from that, this PR is in a great spot and we can merge as soon as the CHANGELOG entry is added and all tests are ✅
|
Done 🚀 |
|
Thank you! This PR definitely improved |
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.
Part of #42753
What?
Refactoring of the Tooltip component is being considered in #42753.
As a first step, I rewritten tests to
@testing-library/react.How?
I straightforwardly replacd
enzymetests with@testing-library/react, with a few exceptions, based on the following post:Testing Instructions
Verify tests pass:npm run test-unit ./packages/components/src/tooltip/test/index.jsVerify tests pass:
npm run test:unit ./packages/components/src/tooltip/test/index.jsScreenshots or screencast