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: Improve act() usage in BlockSwitcher tests #46227

Closed
wants to merge 1 commit into from
Closed

Block Editor: Improve act() usage in BlockSwitcher tests #46227

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
67 changes: 48 additions & 19 deletions packages/block-editor/src/components/block-switcher/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import { render, screen, within } from '@testing-library/react';
import { render, screen, waitFor, within } from '@testing-library/react';
import userEvent from '@testing-library/user-event';

/**
Expand All @@ -15,13 +15,22 @@ import { copy } from '@wordpress/icons';
* Internal dependencies
*/
import { BlockSwitcher, BlockSwitcherDropdownMenu } from '../';
import { act } from 'react-test-renderer';
Copy link
Member

Choose a reason for hiding this comment

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

Oh, we didn't remove all of them yet? 🙂

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 think some of those fixes got lost in the thousand rebases 😅 We'll clean them up, don't worry.


jest.mock( '@wordpress/data/src/components/use-select', () => jest.fn() );
jest.mock( '../../block-title/use-block-display-title', () =>
jest.fn().mockReturnValue( 'Block Name' )
);

/**
* Returns the first found popover element up the DOM tree.
*
* @param {HTMLElement} element Element to start with.
* @return {HTMLElement|null} Popover element, or `null` if not found.
*/
function getWrappingPopoverElement( element ) {
return element.closest( '.components-popover' );
}

describe( 'BlockSwitcher', () => {
test( 'should not render block switcher without blocks', () => {
useSelect.mockImplementation( () => ( {} ) );
Expand Down Expand Up @@ -211,7 +220,18 @@ describe( 'BlockSwitcherDropdownMenu', () => {
} ),
'[ArrowDown]'
);
await act( () => Promise.resolve() );

const menu = screen.getByRole( 'menu', {
name: 'Block Name',
} );

expect( menu ).not.toBeVisible();
Copy link
Member

Choose a reason for hiding this comment

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

This .not.toBeVisible() check points at another thing a reliable test should watch for. The menu is invisible not because the popover is not yet positioned, but because its appearance is animated with framer-motion. At the very beginning the popover has opacity: 0.

The animation and positioning are completely independent from each other.

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly.


await waitFor( () =>
expect(
getWrappingPopoverElement( menu )
).toBePositionedPopover()
);

expect(
screen.getByRole( 'button', {
Expand All @@ -220,11 +240,8 @@ describe( 'BlockSwitcherDropdownMenu', () => {
} )
).toBeVisible();

const menu = screen.getByRole( 'menu', {
name: 'Block Name',
} );
expect( menu ).toBeInTheDocument();
expect( menu ).not.toBeVisible();
expect( menu ).toBeVisible();
Copy link
Member

Choose a reason for hiding this comment

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

At this moment, if you check it with a snapshot, the opacity is not yet 1, but "only" 0.9994. There is an animation frame loop (in framer-motion) that gradually changes it from 0 to 1. At the moment of this check, i.e., after we're finished waiting for popover positioning, the animation hasn't finished yet.

We should do another wait with:

await waitFor( () => expect(popover).toHaveStyle( { opacity: 1 } );

Or integrate it into toBePositionedPopover? Making it do a toBePositionedAndFullDisplayedPopover?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, I was thinking about that. I think this is a worthy improvement, let me work on it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, this is interesting. I haven't debugged thoroughly yet, but the moment I add the assertion you mentioned, or extend .toBePositionedPopover() to check for opacity: 1, I get the act warnings again!

Copy link
Member

@jsnajdr jsnajdr Dec 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I debugged this and the act() warning is caused by this setHasAnimatedOnce state update:

gutenberg/packages/components/src/popover/index.tsx

Lines 120 to 123 in 5fcf00d

const onAnimationComplete = useCallback(
() => setHasAnimatedOnce( true ),
[]
);

When framer-motion is finishing an animation, it will set the final opacity: 1 style, then will resolve a promise, and then there's a .then( () => props.onAnimationComplete() ) handler. The state update runs one promise tick after the waitFor for opacity: 1 has finished.

@ciampo I'm curious what are we preventing with this hasAnimatedOnce guard? My understanding is that with a <motion.div animate={ { opacity: 1 } }>, the animate animation runs only once, on mount, and then never again. But a code comment suggests that it runs also on "subsequent prop changes"? What kind of change exactly can re-trigger the animate animation to run again?

If Popover didn't have the hasAnimatedOnce state, waiting for .toHaveStyle( { opacity: 1 } ) would work flawlessly.

Copy link
Contributor

@ciampo ciampo Jan 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hey @jsnajdr , if I recall correctly that state variable is in place to avoid the animation from re-running when the placement changes, as this inline comment says:

gutenberg/packages/components/src/popover/index.tsx

Lines 109 to 112 in b120213

// When animating, animate only once (i.e. when the popover is opened), and
// do not animate on subsequent prop changes (as it conflicts with
// floating-ui's positioning updates).
const [ hasAnimatedOnce, setHasAnimatedOnce ] = useState( false );

If I remember correctly, without this flag, the animation would run again if the value of the placement prop (and probably other props too) change.

Feel free to play around with it and remove it if we find a better solution to the problem!

Copy link
Member

Choose a reason for hiding this comment

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

without this flag, the animation would run again if the value of the placement prop (and probably other props too) change.

This is however not true, at least with the version of framer-motion that we're using now. The animate animation runs only on mount, never anywhere else, like on a prop change. When placement changes, then via placementToMotionAnimationProps the value of the animate prop changes, too, but that doesn't restart the animation either.

I verified that when debugging framer-motion back in December, and also experimentally. On this video, the block inserter popover appears with animation (I made it run slower to make it more visible), but then the switches between top and bottom placements are not animated any more:

Screen Capture on 2023-01-03 at 14-05-27

I think we can either remove hasAnimatedOnce completely, or use it just to set the is-positioned class (meaning that the popover has finished positioning and animating).

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me, then! Please do ping me on the PR if you're going to work on it, happy to help with reviewing and testing.

} );

test( 'should simulate a click event, which should call onToggle.', async () => {
Expand Down Expand Up @@ -254,7 +271,18 @@ describe( 'BlockSwitcherDropdownMenu', () => {
expanded: false,
} )
);
await act( () => Promise.resolve() );

const menu = screen.getByRole( 'menu', {
name: 'Block Name',
} );

expect( menu ).not.toBeVisible();

await waitFor( () =>
expect(
getWrappingPopoverElement( menu )
).toBePositionedPopover()
);

expect(
screen.getByRole( 'button', {
Expand All @@ -263,11 +291,7 @@ describe( 'BlockSwitcherDropdownMenu', () => {
} )
).toBeVisible();

const menu = screen.getByRole( 'menu', {
name: 'Block Name',
} );
expect( menu ).toBeInTheDocument();
expect( menu ).not.toBeVisible();
expect( menu ).toBeVisible();
} );

test( 'should create the transform items for the chosen block.', async () => {
Expand All @@ -285,14 +309,19 @@ describe( 'BlockSwitcherDropdownMenu', () => {
expanded: false,
} )
);
await act( () => Promise.resolve() );

const menu = screen.getByRole( 'menu', {
name: 'Block Name',
} );

await waitFor( () =>
expect(
getWrappingPopoverElement( menu )
).toBePositionedPopover()
);

expect(
within(
screen.getByRole( 'menu', {
name: 'Block Name',
} )
).getByRole( 'menuitem' )
within( menu ).getByRole( 'menuitem' )
).toBeInTheDocument();
} );
} );
Expand Down