Block Editor: Unmount after each MediaReplaceFlow test
#45736
Closed
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
tyxla
added
[Type] Code Quality
tyxla
requested review from
Mamaduka,
youknowriad,
mirka,
jsnajdr,
ciampo,
chad1008 and
brookewp
|
|
|
Size Change: +6.57 kB (+1%) Total Size: 1.3 MB
ℹ️ View Unchanged
|
Mamaduka
reviewed
Nov 14, 2022
There was a problem hiding this comment.
Based on the docs, cleanup should be automatic when using the Jest framework.
Any idea why it is failing for us?
That's true, The trick is that the test and The |
Yes, definitely. At this moment these fixes are still experimental though. We're in the process of figuring out what the best practices actually are. We'll know better after we've fixed a larger sample of tests with diverse scenarios. Then we can write down authoritative instructions. The manual synchronous |
It has some effect because it makes the tests green, but I'm afraid it's another accidental fix where |
tyxla
changed the title
MediaReplaceFlow testsMediaReplaceFlow test
|
Thanks for the feedback, @Mamaduka, and @jsnajdr! I agree with the rationale to prefer unmounting vs just cleaning up. Unmounting is much more specific in this case, and it's usually immediately clear that it will cancel any running effects. I've thus updated the tests to Noting that my testing demonstrates that just adding Ready for another look 👀 |
jsnajdr
reviewed
Nov 17, 2022
|
|
||
| // Unmount the UI synchronously so that any async effects, like the on-mount focus | ||
| // that shows and positions a tooltip, are cancelled right away and never run. | ||
| unmount(); |
There was a problem hiding this comment.
For any test that's async, and contains await statements, typically user-event calls like await user.click(), this is unsound. The tests are green only because the test function starts winning a microtask race with the floating-ui's computePosition.
The popover is opened by the very first click in the test, on the "Replace" button. Then there are four more awaits before the unmount(). Originally, the unmount() would be done only a few microtasks later, inside afterEach. Now it happens a few microtask earlier.
Meanwhile, the "Replace" button triggered another async function with many await statements, the popover's computePosition(). If it starts winning the race again, the test will start failing.
One way how this fixed test can be made to fail again is to simply replace fake timers with real ones. Then every await.click will internally do a setTimeout( 0 ), and that guarantees that the computePosition task, which doesn't do any timeouts, just microtasks, will always win and fail the test.
There was a problem hiding this comment.
I understand your point, but this is likely true for many other tests. I believe we could make a compromise and go ahead to ship this, unblocking the upgrade.
Perhaps we could add a comment that it needs a follow-up and properly better cleanup in the component itself? From what you explained, it seems like it's an issue that actually spreads beyond the test itself.
WDYT?
There was a problem hiding this comment.
You can merge the PR and postpone the problem to be solved later, but I think it's suboptimal.
MediaReplaceFlow could be a great opportunity to re-introduce the .toBePositioned() helper created in #45587. Clicking on the "Replace" button actually shows a popover and we're actively working with it:
The tests would then be like:
- Render the component.
- Click the "Replace" button.
- Wait for the popover to be open and positioned.
- Do the test-specific work on the popover: check for presence of menu items, edit the media URL etc.
|
This was closed because we chose to go with #45863 as an alternative. |
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?
This PR updates the
MediaReplaceFlowtests to add unmounting after each one, to cancel any remaining async effects.Why?
Those tests are failing in React 18, see #45235. With the unmount, they'll pass both in React 17 and React 18.
How?
We're simply adding an RTL
unmount()at the end of each test.Testing Instructions
npm run test:unit packages/block-editor/src/components/media-replace-flow/test/index.js