BorderBoxControl test: wait for color picker popover to appear - #46632
Open
jsnajdr wants to merge 2 commits into
Open
BorderBoxControl test: wait for color picker popover to appear#46632jsnajdr wants to merge 2 commits into
jsnajdr wants to merge 2 commits into
Conversation
|
Size Change: +417 B (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
Member
Author
|
It's interesting that the "Split view rendering should omit style options" test takes 3.5s to finish: And that makes it time out sometimes, because the Jest limit is 5s per test. My initial profiling shows that it's some big inefficiency inside |
tyxla
reviewed
Dec 19, 2022
|
|
||
| for ( let i = 0; i < colorButtons.length; i++ ) { | ||
| // Click on the color button to show the color picker popover. | ||
| await user.click( colorButtons[ 0 ] ); |
Member
There was a problem hiding this comment.
Did you mean:
Suggested change
| await user.click( colorButtons[ 0 ] ); | |
| await user.click( colorButtons[ i ] ); |
| assertStyleOptionsMissing(); | ||
| await user.click( colorButtons[ 3 ] ); | ||
| // Click again to hide the popover. | ||
| await user.click( colorButtons[ 0 ] ); |
Member
There was a problem hiding this comment.
Same as above:
Suggested change
| await user.click( colorButtons[ 0 ] ); | |
| await user.click( colorButtons[ i ] ); |
|
|
||
| function assertStyleOptionsMissing() { | ||
| const styleLabel = screen.queryByText( 'Style' ); | ||
| const solidButton = screen.queryByRole( 'button', { |
Member
There was a problem hiding this comment.
We should be able to shave ~8-10% of the test time by reducing the number of queries to look for style buttons. So, changing this:
const solidButton = screen.queryByRole( 'button', {
name: 'Solid',
} );
const dashedButton = screen.queryByRole( 'button', {
name: 'Dashed',
} );
const dottedButton = screen.queryByRole( 'button', {
name: 'Dotted',
} );
expect( styleLabel ).not.toBeInTheDocument();
expect( solidButton ).not.toBeInTheDocument();
expect( dashedButton ).not.toBeInTheDocument();
expect( dottedButton ).not.toBeInTheDocument();
to:
const styleButton = screen.queryByRole( 'button', {
name: /(Solid)|(Dashed)|(Dotted)/,
} );
expect( styleButton ).not.toBeInTheDocument();
This exists in another test, so we could improve it there as well.
Comment on lines
+301
to
+305
| await waitFor( () => | ||
| expect( | ||
| getWrappingPopoverElement( colorPickerButton ) | ||
| ).toBePositionedPopover() | ||
| ); |
Member
There was a problem hiding this comment.
This seems to be the most expensive part - taking more or less 1/3 of the test time. Maybe this is a signal that the optimization should be done with the animation itself in the popover component? Should we try massaging the waitFor options (see https://testing-library.com/docs/dom-testing-library/api-async#waitfor)? Or perhaps when we use fake timers we should disable animations - not something I'd recommend TBH.
Comment on lines
+298
to
+300
| const colorPickerButton = screen.getByRole( 'button', { | ||
| name: 'Custom color picker.', | ||
| } ); |
Member
There was a problem hiding this comment.
I've noticed that queries themselves tend to be slow. But making hidden: true in order to disable additional work to exclude invisible elements makes this twice faster.
I've ran this:
console.time( 'without hidden' );
const colorPickerButton = screen.getByRole( 'button', {
name: 'Custom color picker.',
} );
console.timeEnd( 'without hidden' );
console.time( 'with hidden' );
screen.getByRole( 'button', {
name: 'Custom color picker.',
hidden: true,
} );
console.timeEnd( 'with hidden' );
and got these results:
console.time
without hidden: 89 ms
at Object.<anonymous> (packages/components/src/border-box-control/test/index.js:304:13)
console.time
with hidden: 42 ms
at Object.<anonymous> (packages/components/src/border-box-control/test/index.js:311:13)
console.time
without hidden: 98 ms
at Object.<anonymous> (packages/components/src/border-box-control/test/index.js:304:13)
console.time
with hidden: 37 ms
at Object.<anonymous> (packages/components/src/border-box-control/test/index.js:311:13)
console.time
without hidden: 70 ms
at Object.<anonymous> (packages/components/src/border-box-control/test/index.js:304:13)
console.time
with hidden: 36 ms
at Object.<anonymous> (packages/components/src/border-box-control/test/index.js:311:13)
console.time
without hidden: 71 ms
at Object.<anonymous> (packages/components/src/border-box-control/test/index.js:304:13)
console.time
with hidden: 34 ms
at Object.<anonymous> (packages/components/src/border-box-control/test/index.js:311:13)
So adding hidden: true for all queries here should also improve the situation.
That being said, I wonder if we should enable it globally:
https://testing-library.com/docs/dom-testing-library/api-configuration#defaulthidden
That's likely going to cause some issues in tests where we assume that the hidden elements will be excluded. But it might end up making the tests run faster.
WDYT?
Member
Author
There was a problem hiding this comment.
Seems that anything that checks visibility, be it getByRole with hidden: true or the popover wait (but does it really check visibility of anything?) is ridiculously show. That's because there's plenty of window.getComputedStyle() calls, and each takes approx 30ms. That's a lot of time for something that traverses a tree and looks for object properties. In the profiler, flame chart view like this is typical:
We can "optimize" our tests to avoid visibility tests, but having to do that is really silly, the test author shouldn't worry about that at all.
Member
There was a problem hiding this comment.
I agree. Ideally, the testing framework should be responsible for that, and since testing-library doesn't do that, we can enable it for the entire project as I linked above. While that's not perfect, it could at least solve the "the test author shouldn't worry about that at all." problem to some degree.
Member
Author
|
Pushed a commit implementing @tyxla's suggestions: use the Before enabling |
Member
|
I wonder if we can find a good substitute for |
Member
|
In any case, the performance of |
Member
Author
Thanks for finding and posting the link 🙂
|
Member
|
FWIW, despite the optimizations, that test seems to still be timing out (as seen in the latest build). Locally, on |
Member
Author
|
Did some analysis of The |
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
2 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.
Cleans up the
BorderBoxControlunit tests where we're testing the "Style" section of the color picker popover:Namely, that it doesn't appear when disabled with the
enableStyle={ false }prop. This test needs to wait for popover positioning in order to avoidact()warnings.I'm also refactoring the "split view" test to use a
forloop.