Navigation: Add warning test #45207
Merged
Navigation: Add warning test #45207
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
scruffian
added
[Block] Navigation
|
Size Change: 0 B Total Size: 1.33 MB ℹ️ View Unchanged
|
talldan
reviewed
Oct 27, 2022
| */ | ||
| const { test, expect } = require( '@wordpress/e2e-test-utils-playwright' ); | ||
|
|
||
| test.describe( |
There was a problem hiding this comment.
It'd be great to wrap all the tests in a top level test.describe( 'Navigation block', () => {} ) description, so that the console generates the right output.
Comment on lines
21
to
33
| await editor.page.locator( '[aria-label="List View"i]' ).click(); | ||
| // Click the Navigation block expander, we need to use force because it has aria-hidden set to true. | ||
| await editor.page | ||
| .locator( | ||
| `//a[.//span[text()='Navigation']]/span[contains(@class, 'block-editor-list-view__expander')]` | ||
| ) | ||
| .click( { force: true } ); | ||
| // Find the Page list selector inside the navigation block. | ||
| const pageListSelector = await editor.page.locator( | ||
| `//table[contains(@aria-label,'Block navigation structure')]//a[.//span[text()='Page List']]` | ||
| ); | ||
|
|
||
| expect( pageListSelector ).toBeTruthy(); |
There was a problem hiding this comment.
I personally don't think there's any need to check List View here, checking the editor content is enough, and there could be (or are) separate tests for List View checking that its content matches the block editor content. It'll also help the tests be less flaky if there are fewer steps.
|
|
||
| // Find the warning message | ||
| const warningMessage = await editor.page.locator( | ||
| 'class=wp-navigation-block', |
There was a problem hiding this comment.
It's be great to use role selectors where possible over class names—it helps ensure the UI has accessible markup.
For the block it'd be role=document[name="Block: Navigation"i] (the document role for blocks is a bit unusual, but that was apparently necessary). The easiest way to get the right values is to inspect the element in the browser and check the accessibility tab's computed properties.
There's also a shorter way of selecting text role=document[name="Block: Navigation"i] >> text="Navigation menu has been deleted or is unavailable.". (there is a subtle difference, this will select the text node, and hasText selects the navigation block, but I don't think the difference matters here).
I think you should be able to shorten the whole thing to this:
await expect( `role=document[name="Block: Navigation"i] >> text="Navigation menu has been deleted or is unavailable." ).toBeVisible();
There was a problem hiding this comment.
Everything @talldan said 😆, plus a couple of things:
- You can use
pagedirectly rather than referencingeditor.page.editor.pageisn't supposed to be a public API and might be marked private in the future. - Locator doesn't return a promise, so no need to
awaitit. - Playwright deletes the doc for role selectors 😞 , but thankfully the alternative
getByRoleis basically the same thing but more friendly if you're familiar with testing-library's API.
Hence, the whole thing could be:
const warningMessage = page.getByRole( 'document', { name: 'Block: Navigation' } )
.getByText( 'Navigation menu has been deleted or is unavailable.' );There was a problem hiding this comment.
page didn't work ( ReferenceError: page is not defined) but editor.canvas does, which is what we use in other other test.
There was a problem hiding this comment.
In order to use page you'll have to specify it in the test callback parameter:
test( '...', async ( { page } ) => {But yeah, since that now post editor is rendered inside an iframe we can just use editor.canvas instead.
scruffian
force-pushed
the
add/navigation-warning-test
branch
from
8d30d04 to
8916e2b
Compare
kevin940726
reviewed
Feb 6, 2023
| 'Navigation menu has been deleted or is unavailable.', | ||
| } | ||
| ); | ||
| expect( warningMessage ).toBeTruthy(); |
There was a problem hiding this comment.
Also, locator is always truthy as it always returns a locator instance. Instead, we should do something like toBeVisible().
await expect( warningMessage ).toBeVisible();It also follows more closely to how users interpret it too.
scruffian
force-pushed
the
add/navigation-warning-test
branch
from
0c3af1c to
79b0b5b
Compare
scruffian
commented
Feb 9, 2023
| @@ -146,3 +146,39 @@ class NavigationBlockUtils { | |||
| ); | |||
| } | |||
| } | |||
|
|
|||
| test.describe( 'Navigation block', () => { | |||
There was a problem hiding this comment.
We should wrap all tests in this, in another PR!
|
I think this is ready for another review. |
|
Flaky tests detected in b198fc8261d5a58c6af90e121fa17a880e53bfa1. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4143816138
|
scruffian
force-pushed
the
add/navigation-warning-test
branch
from
b198fc8 to
5a01992
Compare
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.
This adds a test to ensure that the user gets a warning message when the ref of their block points to an unknown wp_navigation menu.
Fixes #45204
Testing instructions:
Run npm run test:e2e:playwright -- editor/blocks/navigation.spec.js