Navigation: e2e tests: default to classic menu #47867
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
|
Size Change: +11.9 kB (+1%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 4d4d736. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4162741815
|
MaggieCabrera
force-pushed
the
e2e-default-only-menu
branch
from
c39ac92
to
6e73320
Compare
Co-authored-by: Kai Hao <kevin830726@gmail.com>
MaggieCabrera
force-pushed
the
e2e-default-classic-menu
branch
from
9599509
to
6f78851
Compare
MaggieCabrera
requested review from
scruffian,
getdave,
ajlende and
draganescu
MaggieCabrera
added
[Type] Automated Testing
scruffian
reviewed
Feb 10, 2023
| title: string; | ||
| content: string; | ||
| } | ||
| export interface Menu { |
There was a problem hiding this comment.
Should we use the name Navigation?
There was a problem hiding this comment.
I chose menu because it is used both for classic and navigation menus, but I think we can go with NavigationMenu too.
scruffian
reviewed
Feb 10, 2023
| export async function createClassicMenu( this: RequestUtils, name: string ) { | ||
| const menuItems = [ | ||
| { | ||
| title: 'Home', |
There was a problem hiding this comment.
This is quite a generic name. Is it possible that the test could pass but because it found a different navigation with a "Home" option? Maybe its safer to use a less generic name.
There was a problem hiding this comment.
Makes sense. This function was way more complicated on the puppeteer tests, so I made it much simpler for this test, and maybe we can refactor it later if it's useful for other tests too.
There was a problem hiding this comment.
FYI I've found this to be a problem in the past. I agree with Ben it should use a page name that is unusual and quite long.
There was a problem hiding this comment.
I changed it to "Custom link", shall I make it longer?
There was a problem hiding this comment.
That should be good enough 👍
scruffian
reviewed
Feb 10, 2023
scruffian
approved these changes
Feb 10, 2023
There was a problem hiding this comment.
This works well and looks good to me. It would be good to get a review from someone like @kevin940726 who knows a bit more than I do...
kevin940726
reviewed
Feb 13, 2023
| }, | ||
| } ); | ||
|
|
||
| if ( menuItems?.length ) { |
There was a problem hiding this comment.
I'm not sure why this is needed? Isn't menuItems statically defined above?
| /** | ||
| * Create a navigation menu | ||
| * | ||
| * @param {Object} menuData navigation menu post data. |
There was a problem hiding this comment.
Nit: Prefer TS types than JSDoc types. I think we can just omit the type here.
* @param menuData navigation menu post data.| path: `/wp/v2/navigation/`, | ||
| } ); | ||
|
|
||
| if ( navMenus?.length ) { |
There was a problem hiding this comment.
Nit: It's always going to return an array so let's remove the optional chaining here :)
| path: `/wp/v2/menus/`, | ||
| } ); | ||
|
|
||
| if ( classicMenus?.length ) { |
| ] ); | ||
|
|
||
| // Check the block in the canvas. | ||
| await editor.page.pause(); |
|
|
||
| // Check the block in the frontend. | ||
| await navBlockUtils.selectNavigationItemOnFrontend( 'Custom link' ); | ||
| await editor.page.pause(); |
| ); | ||
| async selectNavigationItemOnFrontend( name ) { | ||
| await this.page.goto( '/' ); | ||
| await this.editor.page.pause(); |
| ...menuData, | ||
| }, | ||
| } ); | ||
| async selectNavigationItemOnCanvas( name ) { |
There was a problem hiding this comment.
Prefer inlining one-liner assertions like this, makes the test a bit easier to read. selectNavigationItemOnCanvas doesn't really select anything here anyway 😆 .
| path: `/wp/v2/navigation/${ menu.id }?force=true`, | ||
| } ) ) | ||
| ); | ||
| async selectNavigationItemOnFrontend( name ) { |
DaisyOlsen
changed the title
|
Hi, @MaggieCabrera The new test has been failing from time to time. See #48033. |
|
It's also mentioned in #48071 (comment). I'm sure it's already on @MaggieCabrera 's list. But we're also happy to help if needed! ❤️ |
|
Thanks for the reminder! do you have any ideas to make it more consistent? we could go without checking if the link is visible in the editor I suppose. I don't know how we could check it any other way, we are already looking at the selector in the canvas. |
|
It seems like it just takes a long time to finish. Could possibly be solved by just increasing the timeout for the assertion. The real question is why it is taking so long to load the navigation menu? |
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?
This is part of the effort to re work all the navigation block e2e tests and migration to playwright. This is one of the user stories described in #45199
This test checks that when only one classic menu exists, inserting a Navigation block will default correctly to using said menu.
Testing Instructions
Run
npm run test:e2e:playwright -- editor/blocks/navigation.spec.js