Tooltip: refine existing tests #48397
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
brookewp
commented
Feb 24, 2023
brookewp
commented
Feb 24, 2023
|
Size Change: 0 B Total Size: 1.34 MB ℹ️ View Unchanged
|
|
Flaky tests detected in 4f61cef. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4473818106
|
tyxla
reviewed
Feb 24, 2023
Comment on lines
216
to
222
| const buttonRect = button.getBoundingClientRect(); | ||
| // eslint-disable-next-line testing-library/no-container, testing-library/no-node-access | ||
| const eventCatcher = container.querySelector( '.event-catcher' ); | ||
| const eventCatcherRect = eventCatcher.getBoundingClientRect(); | ||
| expect( buttonRect ).toEqual( eventCatcherRect ); | ||
|
|
||
| await user.hover( eventCatcher ); |
There was a problem hiding this comment.
I'm not sure the new version of the test reflects what this was doing. Thoughts?
There was a problem hiding this comment.
I maybe be misunderstanding, or perhaps the test is mislabelled and an additional test is needed? When I see the text should show tooltip when an element is disabled, I assumed that is all that needs to be checked.
And with the new tooltip, we won't have the 'event catcher' so then the test wouldn't translate to the new one. So maybe it would be best to separate this and then leave the implementation detail for a separate test that won't be used for the new tooltip?
There was a problem hiding this comment.
What you're suggesting makes sense, especially in the context of the new tooltip that doesn't have the event catcher.
There was a problem hiding this comment.
To clarify, do you mean the suggestion of separating the test into two: one for checking a disabled element and a new one to test event handling?
Or the original change?
There was a problem hiding this comment.
I meant the original change. Feel free to do as you prefer with regards to separating the change. If we're getting rid of this version of the component soon, it might make sense to have 2 separate tests and then we'd just remove the event catcher one with the new version of the component.
brookewp
commented
Feb 25, 2023
| @@ -23,171 +28,136 @@ describe( 'Tooltip', () => { | |||
| describe( '#render()', () => { | |||
There was a problem hiding this comment.
As a last step, I think we could remove the nested describe here
brookewp
added
[Type] Enhancement
ciampo
reviewed
Mar 2, 2023
Comment on lines
252
to
258
| await waitFor( | ||
| () => | ||
| expect( | ||
| screen.queryByText( 'tooltip text' ) | ||
| ).not.toBeInTheDocument(), | ||
| { timeout: MOUSE_LEAVE_DELAY } | ||
| ); |
There was a problem hiding this comment.
I wonder if this assertion is actually testing what we want.
Since the tooltip shouldn't be visible , this waitFor call would possibly immediately "pass", instead of performing the check at the end of the timeout time.
Basically, using waitFor works well when waiting for a condition to happen asynchronously — but it's likely not the right tool to test for a condition NOT to change for a specific amount of time.
@tyxla , do you have any thoughts? If my assumptions are correct, we may have to rewrite a few assertions throughout this file.
There was a problem hiding this comment.
Yep, I've taken a look, and it seems like timeout doesn't work the way the current version of the assertion assumes.
The timeout works like this: it will run on every X ms (X being the specified interval, 50ms by default), and if the assertion passes on the first run, it will resolve.
That means that we should instead actually set a timeout, and perform an assertion after that timeout.
Alternatively, we could use waitForElementToBeRemoved(), which should better express what we're aiming to do here, without having to specifically play with setting up intervals and timeouts.
There was a problem hiding this comment.
Thank you both! That makes sense why waitFor doesn't work well in this situation, where we are testing that something shouldn't happen after a set time.
However, I am unsure if I understand why waitForElementToBeRemoved() would work in this case. The tooltip should never be rendered because the mouse moved away from the anchor before it's set delay. So how would waiting for it to be removed from the DOM work here?
There was a problem hiding this comment.
Yep, you're right, waitForElementToBeRemoved() won't do the job, because it's already removed and will immediately pass.
I guess we can do the following then:
- Add a manual timeout:
await new Promise( ( resolve ) => setTimeout( resolve, MOUSE_LEAVE_DELAY ) );
- Add the
.not.toBeInTheDocument()assertion afterwards.
How does that sound?
There was a problem hiding this comment.
Sounds great, thank you!
brookewp
commented
Mar 16, 2023
| const user = userEvent.setup( { | ||
| advanceTimers: jest.advanceTimersByTime, | ||
| } ); | ||
| const eventCatcherRect = eventCatcher.getBoundingClientRect(); |
There was a problem hiding this comment.
Instead of creating another test as we discussed, I moved the implementation details to this test which is also testing the event catcher for a disabled button. Thoughts?
brookewp
commented
Mar 16, 2023
| shortcut={ { | ||
| display: 'shortcut text', | ||
| ariaLabel: 'shortcut label', | ||
| display: '⇧⌘,', |
There was a problem hiding this comment.
I changed the values of the shortcut object to test the shortcut's purpose, which is to render keyboard shortcuts and corresponding aria-labels for screenreaders.
brookewp
force-pushed
the
update/legacy-tooltip
branch
from
0f11fc4
to
4f61cef
Compare
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?
Refining the existing tooltip tests (and likely adding a few more but this is still a WIP)
Why?
In preparation for a new tooltip component #48222, I wanted to refine and improve the existing tests for the legacy tooltip. As well as add any additional tests that are missing / would be beneficial.
How?
focus()withuserEvent.tab()to closer mimic user behaviouract()and fake timersTesting Instructions
Run
npm run test:unit packages/components/src/tooltip/test/index.jsand ensure tests are passing