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

Components: Remove unnecessary act() from Popover tests #47690

Merged
merged 2 commits into from
Feb 3, 2023
Merged

Components: Remove unnecessary act() from Popover tests #47690

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
edf66c2
Components: Remove unnecessary act() from Popover tests
tyxla Feb 2, 2023
a9bd72b
No need to await for the focusing
tyxla Feb 3, 2023
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
49 changes: 28 additions & 21 deletions packages/components/src/popover/test/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
/**
* External dependencies
*/
import { act, render, screen } from '@testing-library/react';
import type { CSSProperties, ReactElement } from 'react';
import { render, screen, waitFor } from '@testing-library/react';
import type { CSSProperties } from 'react';

/**
* WordPress dependencies
Expand Down Expand Up @@ -91,25 +91,21 @@ const ALL_POSITIONS_TO_EXPECTED_PLACEMENTS: PositionToPlacementTuple[] = [

describe( 'Popover', () => {
describe( 'Component', () => {
// Render UI and then wait for the `floating-ui` effects inside `Popover` to finish running
// See also: https://floating-ui.com/docs/react-dom#testing
async function renderAsync( ui: ReactElement ) {
const view = render( ui );
await act( () => Promise.resolve() );
return view;
}

describe( 'basic behavior', () => {
it( 'should render content', async () => {
await renderAsync( <Popover>Hello</Popover> );
render( <Popover>Hello</Popover> );

expect( screen.getByText( 'Hello' ) ).toBeInTheDocument();
await waitFor( () =>
expect( screen.getByText( 'Hello' ) ).toBeVisible()
);
} );

it( 'should forward additional props to portaled element', async () => {
await renderAsync( <Popover role="tooltip">Hello</Popover> );
render( <Popover role="tooltip">Hello</Popover> );

expect( screen.getByRole( 'tooltip' ) ).toBeInTheDocument();
await waitFor( () =>
expect( screen.getByRole( 'tooltip' ) ).toBeVisible()
);
} );
} );

Expand All @@ -129,32 +125,43 @@ describe( 'Popover', () => {
);
};

await renderAsync(
render(
<PopoverWithAnchor>Popover content</PopoverWithAnchor>
);

expect(
screen.getByText( 'Popover content' )
).toBeInTheDocument();
await waitFor( () =>
expect(
screen.getByText( 'Popover content' )
).toBeVisible()
);
} );
} );

describe( 'focus behavior', () => {
it( 'should focus the popover by default when opened', async () => {
await renderAsync(
render(
<Popover data-testid="popover-element">
Popover content
</Popover>
);

expect( screen.getByTestId( 'popover-element' ) ).toHaveFocus();
const popover = screen.getByTestId( 'popover-element' );

await waitFor( () => expect( popover ).toBeVisible() );
await waitFor( () => expect( popover ).toHaveFocus() );
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the double await necessary? Should we add an inline comment with the explanation?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not necessary actually. I've cleaned the second one up.

} );

it( 'should allow focus-on-open behavior to be disabled', async () => {
await renderAsync(
render(
<Popover focusOnMount={ false }>Popover content</Popover>
);

await waitFor( () =>
expect(
screen.getByText( 'Popover content' )
).toBeVisible()
);

expect( document.body ).toHaveFocus();
} );
} );
Expand Down