Add e2e testing for experimental nav menu deletion #38955

chad1008 · 2022-02-21T13:29:16Z

Description

Follow up to #37492

This PR adds e2e tests for menu deletion, and the newly migrated ConfirmDialog, in the experimental nav editor. It tests:

canceling the deletion
confirming the deletion when there are no other existing menus
confirming the deletion when there still are other existing menus

The above are all tested twice, once for large and once for small viewports.

The confirmation test happens in two different contexts because the flow is different. With no other menus, we're shown the "Create your first menu" prompt, while if additional menus remain after deletion, we should simply see one of the other menus instead.

Edited to add:
I initially forgot to mention, I do see frequent (not constant, but on a lot of test runs) warnings from Jest:

Jest did not exit one second after the test run has completed.

This usually means that there are asynchronous operations that weren't stopped in your tests. Consider running Jest with --detectOpenHandles to troubleshoot this issue.

In troubleshooting, I've found this comes up inconsistently with existing tests in the suite as well. The more tests you run the more likely you are to see it, so with this PR's six tests it comes up a lot.

I think it's being triggered by something in the timing of the createMenu() method's API call, but that's just a theory. Either way it feels like something best pursued in a separate PR.

Testing Instructions

This is an experimental feature, so the main suite is currently skipped. Remove the .skip from packages/e2e-tests/specs/experiments/navigation-editor.test.js line 181.
On line 683 of the same file, add a .only to the describe call. This is both to speed up testing and also to skip over issues that appear to be present in some of the existing tests.
Run the new tests: npx wp-scripts test-e2e --config packages/e2e-tests/jest.config.js packages/e2e-tests/specs/experiments/navigation-editor.test.js
All 6 tests should pass

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
I've tested my changes with keyboard and screen readers.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).
I've updated related schemas if appropriate.

cc @ciampo @fullofcaffeine

ciampo

Thank you for working on this, @chad1008 !

I left a few comments, although I'd prefer having another pair or eyes more experienced with e2e tests in this part of repo also have a look — cc;ing @getdave @talldan @Mamaduka who all have worked on this file before

Maybe they may also have an answer to the question that you raise in this PR's description re. the Jest did not exit one second after the test run has completed. warning

ciampo · 2022-02-23T20:07:35Z

packages/e2e-tests/specs/experiments/navigation-editor.test.js

+				const menuActionsDropdown = await page.waitForXPath(
+					`//*[contains(@class,"edit-navigation-menu-actions")]//h2[text()="${ menuName }"]`


I was about to suggest that, instead of a classname, we'd look for the dropdown's toggle button instead (a button with an aria-label of Switch menu, according to the code), but as I inspected the page locally, I realised the aria-label was not there.

That's because the label should be applied as a prop of DropdownMenu (i.e <DropdownMenu label={ __('Switch Menu') } ....), and not as part of toggleProps.

Once this PR is merged, we could open a follow-up PR where we fix this bug and we update the e2e introduced here to use the selector that I suggested above — WDYT ?

Oh awesome! I was also looking for a non-class approach to this and didn't see anything I could point to... Thanks for sharing the context on why that was happening!

A followup PR sounds great, I'll put it on my to-do list once this merges!

packages/e2e-tests/specs/experiments/navigation-editor.test.js

ciampo · 2022-02-23T20:18:24Z

packages/e2e-tests/specs/experiments/navigation-editor.test.js

+				const menuElementHandles = await page.$x(
+					'//*[@role="group"]//*[@role="menuitemradio"]/span'
+				);
+				const existingMenus = [];
+				for ( const elem of menuElementHandles ) {
+					const elemName = await elem.getProperty( 'textContent' );
+					const existingMenuName = await elemName.jsonValue();
+					existingMenus.push( existingMenuName );
+				}
+				expect( existingMenus.includes( `${ menuName }` ) ).toBe(
+					false
+				);


This part feels very "implementation details"-oriented.. maybe a better way to test for this is to (if I understand the purpose of this code):

Create both menu1 and menu2

Check that there is a dropdown entry (menuitemradio) for both menus

Delete menu2

Check that there is still a dropdown entry for the menu1

Check for menu2 entry, expect the result to be null (or whatever the result should be when an element can't be found in the page)

Good suggestion. I pushed 26fbcc5. Unfortunately waitForXPath() doesn't return anything when it fails to find an element, it just times out, throws an error and stops the test.

$x() is meant to return an array of matching ElementHandles, which I could then try to call toContain() against... but I had some trouble implementing it so I wound up falling back to waitForXPath() and parsing TimeOut errors as null to serve as an indicator that the element isn't present. What do you think?

I'm not an expert about puppetteer APIs, but overall it seems a good enough approach.

My only comment, at this point, is regarding the verbosity of these checks — have you searched if there's already a shared util for performing such a check?

Alternatively, we could simply refactor the logic into a getMenuItem( menuItemName ) function, on top of my head something like:

const getMenuItem = async ( menuItemName ) => await page .waitForXPath( `//*[@role="group"]//*[@role="menuitemradio"]/span[text()="${ menuItemName }"]` ) .catch( ( error ) => { if ( error.name !== 'TimeoutError' ) { throw error; } else { return null; } } );

I haven't seen any similar shared utilities to easily check for the existence of an element, but I like your refactoring suggestion, thank you! pushed f6a2f44 :)

packages/e2e-tests/specs/experiments/navigation-editor.test.js

talldan · 2022-02-25T03:21:20Z

Sorry, I just saw this. I think all the Navigation Editor end to end tests are skipped right now as the editor is mostly broken state. It probably shouldn't be something users can enable right now.

I don't think these tests will run while they're nested in the top level describe. Apologies if that means there has been wasted effort!

It does look like the menu creation and deletion process still works, so un-nesting specific tests from that top level describe block could be an option.

chad1008 · 2022-02-25T12:06:08Z

No worries @talldan! I saw the main describe was skipped (which makes sense for an experimental UI) but figured it'd make sense to have tests prepped for the new ConfirmDialog anyway... I didn't want someone else having to come along and create tests from scratch for something I'd migrated before the feature was ready to go live, but I totally recognize they may need some tweaking depending on how the new navigation eventually finalizes 😄

In terms of un-nesting, I'm open to whatever more experienced Gutenberg contributors think there. Is it better to keep this nested and skipped until the UI actually starts being used? Or unnest them so they'll get incrementally improved as things change around them?

ciampo · 2022-02-25T14:47:38Z

If I understand what @talldan intended, I think it would be ok to un-nest (i.e. move them under a separate describe) all tests specific to menu creation and deletion — from a quick look, this could be a good list:

all of the test added by this PR (under the 'Delete menu button' describe)
the first 3 tests:
- 'allows creation of a menu when there are no current menu items'
- 'allows creation of a menu when there are existing menu items'
- 'displays the first created menu when at least one menu exists'

What do you think ?

chad1008 · 2022-02-28T12:35:10Z

Sounds good to me. I found there was one failing test and several obsolete snapshots in the menu creation tests, so I've left them where they are for now, and separated out the new tests for the delete menu button. 2c48c25

ciampo

LGTM 🚀 Thank you for working on this!

Mamaduka · 2022-03-23T06:50:27Z

@chad1008, this test seems to be failing a lot lately.

I'm not sure if the menu deletion process broke recently, but it would be great to debug flaky tests.

P.S. Considering that Navigation Editor currently isn't actively developed, we could also skip these tests.

talldan · 2022-03-25T06:30:14Z

I've decided to skip them in #39746. I did have a look to see if I could figure out what the issue is. I'm not sure though.

Potentially it could be related to the 'Theme locations' panel above the Delete button. That panel content loads in dynamically, so the test could be trying to click while the browser is still laying out the sidebar content—the element might be moving out from the position that the test is clicking in.

If we can figure out what it is then we can look at re-enabling them.

chad1008 · 2022-03-29T12:18:32Z

Thanks @talldan. Pausing for now definitely seems like the right move, especially where this is a UI that isn't in use yet. I'll put looking into those failures on my todo list. Your theory about the theme locations sounds solid, I'll start there.

chad1008 requested review from ntwb, nerrad and ajitbohra as code owners February 21, 2022 13:29

ciampo requested review from ciampo and fullofcaffeine February 21, 2022 16:35

ciampo assigned chad1008 Feb 21, 2022

ciampo added [Package] E2E Tests /packages/e2e-tests [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. labels Feb 21, 2022

ciampo reviewed Feb 23, 2022

View reviewed changes

ciampo requested review from getdave, Mamaduka and talldan February 23, 2022 20:22

chad1008 force-pushed the add/experimental-nav-delete-menu-e2e-test branch from 3d3c79a to e7653f3 Compare February 25, 2022 11:50

chad1008 added 6 commits February 28, 2022 07:10

add menu deletion e2e tests

24c47ae

add small viewport tests for menu deletion

f0173ef

improve XPath for elements in top navigation bar

b5391d2

simplify implementation of checking for a deleted menu

0ce0665

increase specifity of the delete menu button xpath

bffacaf

refactor menu item detection into its own function

f6a2f44

chad1008 force-pushed the add/experimental-nav-delete-menu-e2e-test branch from e7653f3 to f6a2f44 Compare February 28, 2022 12:10

unnest menu deletion tests from the main, currently skipped, describe

2c48c25

ciampo approved these changes Mar 1, 2022

View reviewed changes

ciampo merged commit ab64c85 into WordPress:trunk Mar 1, 2022

github-actions bot added this to the Gutenberg 12.8 milestone Mar 1, 2022

chad1008 deleted the add/experimental-nav-delete-menu-e2e-test branch March 15, 2022 13:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add e2e testing for experimental nav menu deletion #38955

Add e2e testing for experimental nav menu deletion #38955

chad1008 commented Feb 21, 2022 •

edited

Loading

ciampo left a comment •

edited

Loading

ciampo Feb 23, 2022

chad1008 Feb 24, 2022

ciampo Feb 23, 2022

chad1008 Feb 25, 2022

ciampo Feb 25, 2022 •

edited

Loading

chad1008 Feb 28, 2022

talldan commented Feb 25, 2022

chad1008 commented Feb 25, 2022

ciampo commented Feb 25, 2022

chad1008 commented Feb 28, 2022

ciampo left a comment

Mamaduka commented Mar 23, 2022

talldan commented Mar 25, 2022

chad1008 commented Mar 29, 2022

		const menuActionsDropdown = await page.waitForXPath(
		`//*[contains(@class,"edit-navigation-menu-actions")]//h2[text()="${ menuName }"]`

Add e2e testing for experimental nav menu deletion #38955

Add e2e testing for experimental nav menu deletion #38955

Conversation

chad1008 commented Feb 21, 2022 • edited Loading

Description

Testing Instructions

Checklist:

ciampo left a comment • edited Loading

Choose a reason for hiding this comment

ciampo Feb 23, 2022

Choose a reason for hiding this comment

chad1008 Feb 24, 2022

Choose a reason for hiding this comment

ciampo Feb 23, 2022

Choose a reason for hiding this comment

chad1008 Feb 25, 2022

Choose a reason for hiding this comment

ciampo Feb 25, 2022 • edited Loading

Choose a reason for hiding this comment

chad1008 Feb 28, 2022

Choose a reason for hiding this comment

talldan commented Feb 25, 2022

chad1008 commented Feb 25, 2022

ciampo commented Feb 25, 2022

chad1008 commented Feb 28, 2022

ciampo left a comment

Choose a reason for hiding this comment

Mamaduka commented Mar 23, 2022

talldan commented Mar 25, 2022

chad1008 commented Mar 29, 2022

chad1008 commented Feb 21, 2022 •

edited

Loading

ciampo left a comment •

edited

Loading

ciampo Feb 25, 2022 •

edited

Loading