Add basic test coverage for Navigation Menu editing mode #56871
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
getdave
added
[Type] Code Quality
|
Size Change: -15 kB (-1%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 76e3ec5. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7193818704
|
scruffian
reviewed
Dec 8, 2023
scruffian
approved these changes
Dec 8, 2023
Co-authored-by: Ben Dwyer <[email protected]>
draganescu
reviewed
Dec 8, 2023
draganescu
reviewed
Dec 8, 2023
There was a problem hiding this comment.
I think we're testing everything in one test:
- that the navigation to focus mode works
- that in focus mode the list viewworks
- that in focus mode the sidebar works as expected
- that the navigation is locked
We can either split into multiple tests or expect less things IMO
- that the navigation block in focus mode
kevin940726
reviewed
Dec 11, 2023
There was a problem hiding this comment.
Personally I like grouping similar assertions into one test in e2e testing as it makes them run faster. We could use the optional expect message to document each assertion. Or we could use test.step if they make sense.
| .getByRole( 'button', { | ||
| name: 'Primary Menu', | ||
| } ) | ||
| .click(); |
There was a problem hiding this comment.
There's a test failure (possibly flaky) here. Could we look into it?
There was a problem hiding this comment.
I think this was resolved by putting a expect on the selector above this one.
I tested by running the test 20 times and saw no failures.
npm run test:e2e:playwright -- -g "Editing Navigation Menus" --repeat-each 20
|
Thanks for the reviews @scruffian @draganescu and @kevin940726 👍
This was intentional. My thinking was that e2e tests are relatively expensive to run and spinning up multiple testing scenarios for asserting something whose setup is nearly identical is unnecessary overhead for a simple smoke test.
TIL. Nice! |
|
I've enabled auto merge here based on @scruffian's 👍 review. I recognise that folks had questions/concerns and I've tried to address most of the them. However, I think the value of shipping this and having some coverage exceeds the value of continuing to iterate here on this PR. We can easily follow up if folks feel strongly about any of the outstanding items. |
kevin940726
reviewed
Dec 13, 2023
| ); | ||
|
|
||
| // Wait for list of Navigations to appear. | ||
| editorSidebar.getByRole( 'heading', { |
There was a problem hiding this comment.
Test still seems flaky. I'll take another look
There was a problem hiding this comment.
Going to see if I can get the flaky test to happen in #57016
artemiomorales
pushed a commit
that referenced
this pull request
Jan 4, 2024
* Smoke test for key behaviours * Update test/e2e/specs/site-editor/navigation-editor.spec.js Co-authored-by: Ben Dwyer <[email protected]> * Use assertion * Prefer role selector * Use steps to break up test --------- Co-authored-by: Andrei Draganescu <[email protected]> Co-authored-by: Ben Dwyer <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Provided some high level tests for the key features that distinguish the Navigation Menu editing mode from editing other content types.
Why?
Guard against regressions such as that which occurred in #56856.
How?
Basic e2e test.
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast