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

Navigator: add more pattern matching tests, refine existing tests #47910

Merged
merged 7 commits into from
Feb 9, 2023

Conversation

ciampo
Copy link
Contributor

@ciampo ciampo commented Feb 9, 2023

What?

Following up the work in #47827, this PR adds more unit test to the suite of Navigator components (and refines some existing ones).

Why?

More tests, more confidence when making changes (especially in preparation for #47883)

How?

  • Added a few tests for the pattern matching using navigator components (on top of existing patternMatch unit tests)
  • Refined tests around focus restoration, to make sure that the correct behaviour is being effectively tests

Testing Instructions

No runtime changes.

Make sure that unit test changes look good, and that tests pass.

@ciampo ciampo self-assigned this Feb 9, 2023
@ciampo ciampo added [Type] Code Quality Issues or PRs that relate to code quality [Package] Components /packages/components labels Feb 9, 2023
@ciampo ciampo marked this pull request as ready for review February 9, 2023 09:17
@@ -98,25 +105,6 @@ function CustomNavigatorButton( {
);
}

function CustomNavigatorButtonWithFocusRestoration( {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in favour of using CustomNavigatorButton everywhere, since today there are basically no differences between the two

@@ -396,8 +428,6 @@ describe( 'Navigator', () => {
} );

it( 'should escape the value of the `path` prop', async () => {
const user = userEvent.setup();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved part of this test to a new test (should restore focus correctly even when the path needs to be escaped). Reviewing commit-by-commit should make this part easier to review.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Great additions. This is going to give us much needed confidence with the goToParent work.

@github-actions
Copy link

github-actions bot commented Feb 9, 2023

Flaky tests detected in dae7b0a.
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/4137292770
📝 Reported issues:

@ciampo ciampo enabled auto-merge (squash) February 9, 2023 09:39
Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

Nice work @ciampo 👍 It's a delight to be reading your test code 🥇

@ciampo ciampo added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Feb 9, 2023
@ciampo ciampo force-pushed the feat/navigator-more-tests branch 2 times, most recently from 447e3cd to 2f3c758 Compare February 9, 2023 14:54
@ciampo ciampo disabled auto-merge February 9, 2023 17:16
@ciampo ciampo merged commit 81bac1d into trunk Feb 9, 2023
@ciampo ciampo deleted the feat/navigator-more-tests branch February 9, 2023 19:05
@github-actions github-actions bot added this to the Gutenberg 15.2 milestone Feb 9, 2023
@ntsekouras ntsekouras added Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta and removed Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Feb 10, 2023
ntsekouras pushed a commit that referenced this pull request Feb 13, 2023
…7910)

* Add one extra button to improve quality of focus restoration tests

* Fix typo

* Add screens with pattern matching to test component

* Add named arguments tests

* Refine invalid HTML attribute tests, split focus restoration part to new test

* Remove reduntant component

* CHANGELOG
@ntsekouras
Copy link
Contributor

Cherry-picked this PR to the wp/6.2 branch.

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.

4 participants