Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Block Editor: Unmount after each MediaReplaceFlow test #45736

Closed
wants to merge 2 commits into from
+20 −4
Closed

Block Editor: Unmount after each MediaReplaceFlow test #45736

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0de905d
Block Editor: Cleanup after MediaReplaceFlow tests
tyxla Nov 12, 2022
c823760
Unmount instead of cleanup()
tyxla Nov 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 20 additions & 4 deletions packages/block-editor/src/components/media-replace-flow/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,22 +34,26 @@ function TestWrapper() {

describe( 'General media replace flow', () => {
it( 'renders successfully', () => {
render( <TestWrapper /> );
const { unmount } = render( <TestWrapper /> );

expect(
screen.getByRole( 'button', {
expanded: false,
name: 'Replace',
} )
).toBeVisible();

// 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();
} );

it( 'renders replace menu', async () => {
const user = userEvent.setup( {
advanceTimers: jest.advanceTimersByTime,
} );

render( <TestWrapper /> );
const { unmount } = render( <TestWrapper /> );

await user.click(
screen.getByRole( 'button', {
Expand All @@ -62,14 +66,18 @@ describe( 'General media replace flow', () => {

expect( uploadMenu ).toBeInTheDocument();
expect( uploadMenu ).not.toBeVisible();

// 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();
} );

it( 'displays media URL', async () => {
const user = userEvent.setup( {
advanceTimers: jest.advanceTimersByTime,
} );

render( <TestWrapper /> );
const { unmount } = render( <TestWrapper /> );

await user.click(
screen.getByRole( 'button', {
Expand All @@ -83,14 +91,18 @@ describe( 'General media replace flow', () => {
name: 'example.media (opens in a new tab)',
} )
).toHaveAttribute( 'href', 'https://example.media' );

// 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();
} );

it( 'edits media URL', async () => {
const user = userEvent.setup( {
advanceTimers: jest.advanceTimersByTime,
} );

render( <TestWrapper /> );
const { unmount } = render( <TestWrapper /> );

await user.click(
screen.getByRole( 'button', {
Expand Down Expand Up @@ -124,5 +136,9 @@ describe( 'General media replace flow', () => {
name: 'new.example.media (opens in a new tab)',
} )
).toHaveAttribute( 'href', 'https://new.example.media' );

// 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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Screenshot 2022-11-17 at 12 26 50

The tests would then be like:

  1. Render the component.
  2. Click the "Replace" button.
  3. Wait for the popover to be open and positioned.
  4. Do the test-specific work on the popover: check for presence of menu items, edit the media URL etc.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, @jsnajdr. I decided to go for it and crafted an alternative solution here: #45863. Please, take a look when you have a chance.

} );
} );