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.

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

Conversation

tyxla
Copy link
Member

@tyxla tyxla commented Feb 2, 2023

What?

This PR cleans up a few unnecessary act() calls and improves some of the Popover tests.

Why?

When preparing our tests to work with React 18 we did a bunch of those act() calls. It's time to clean them up where possible.

How?

  • We're relying on the element to be visible instead of the act call.
  • We're removing the renderAsync() helper and inlining the rendering for clarity and simplicity. IMO it wasn't adding value to the tests, but rather was creating indirection when reading the tests.

Testing Instructions

Verify tests still pass: npm run test:unit packages/components/src/popover/test/index.tsx

Testing Instructions for Keyboard

None

Screenshots or screencast

None

@tyxla tyxla added [Type] Code Quality Issues or PRs that relate to code quality [Package] Components /packages/components labels Feb 2, 2023
@tyxla tyxla requested a review from ajitbohra as a code owner February 2, 2023 13:40
@tyxla tyxla self-assigned this Feb 2, 2023
@github-actions
Copy link

github-actions bot commented Feb 2, 2023

Flaky tests detected in a9bd72b.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4082937127
📝 Reported issues:

Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

Neat!

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.

Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

🚀

@tyxla tyxla merged commit 12791ec into trunk Feb 3, 2023
@tyxla tyxla deleted the refactor/unnecessary-act-popover-tests branch February 3, 2023 10:14
@github-actions github-actions bot added this to the Gutenberg 15.2 milestone Feb 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants