Add nightwatch tests for toolbar

Tests should come back green now after #3255836: Test fails due to Composer 2.2 solved the unrelated test failure.

I think overall this looks good. Totally looking forward to having this test coverage in core. Sorry for basically a nitpick only review:

1. --- /dev/null
   +++ b/core/modules/toolbar/tests/src/Nightwatch/Tests/toolbarBackboneShimTest.js
   

   Should we rename this to something else than BackboneShimTest?

2. +++ b/core/modules/toolbar/tests/src/Nightwatch/Tests/toolbarBackboneShimTest.js
   @@ -0,0 +1,248 @@
   +// Unit tests for the new toolbar api
   

   I think this comment needs to be updated too.

3. +++ b/core/modules/toolbar/tests/src/Nightwatch/Tests/toolbarBackboneShimTest.js
   @@ -0,0 +1,248 @@
   +    // To clear active tab/tray from previous tests
   

   Needs a full stop 😅

4. +++ b/core/modules/toolbar/tests/src/Nightwatch/Tests/toolbarBackboneShimTest.js
   @@ -0,0 +1,248 @@
   +      // Clear escapeAdmin url values.
   

   s/url/URL

5. +++ b/core/modules/toolbar/tests/src/Nightwatch/Tests/toolbarTest.js
   @@ -0,0 +1,375 @@
   +      // To clear active tab/tray from previous tests
   

   Nit: needs a full stop.

6. +++ b/core/modules/toolbar/tests/src/Nightwatch/Tests/toolbarTest.js
   @@ -0,0 +1,375 @@
   +        // Hide the admin menu first, this event is not firing reliably otherwise.
   

   Nit: this line needs to be split

7. +++ b/core/modules/toolbar/tests/src/Nightwatch/Tests/toolbarTest.js
   @@ -0,0 +1,375 @@
   +    // Set user as active tab
   ...
   +    // Check tab and tray are open
   ...
   +    // Set orientation to vertical
   ...
   +    // Check user tab is active
   ...
   +    // Check tray is active and orientation is vertical
   

   These also need a full stop.

Awesome, thank you @nod_! Adding credit for @hooroomoo since they worked on this.

Very happy to see this getting test coverage! This will make future changes to Toolbar much safer. I spotted a few small things:

1. +++ b/core/modules/toolbar/tests/src/Nightwatch/Tests/toolbarApiTest.js
   @@ -0,0 +1,251 @@
   +          const tab = document.querySelector('#toolbar-item-user');
   +          tab.dispatchEvent(new MouseEvent('click', { bubbles: true }));
   

   Shouldn't the click event happen outside the timeout? It seems like the point of the timeout is to provide a small wait after the click event to ensure the expected changes have completed even when the test environment is running a little slow.

2. +++ b/core/modules/toolbar/tests/src/Nightwatch/Tests/toolbarApiTest.js
   @@ -0,0 +1,251 @@
   +        const expectedTrue = {
   +          toolbarModelChangedTab: 'get("activeTab") has expected result',
   +          toolbarModelChangedTray: 'get("activeTray") has expected result',
   +        };
   +        Object.keys(expectedTrue).forEach((property) => {
   +          browser.assert.equal(
   +            result.value[property],
   +            true,
   +            expectedTrue[property],
   +          );
   

   expectedTrue is missing several of the properties from toReturn. For this and other uses of the expectedTrue pattern, include a check that expectedTrue.length === result.value.length, and the tests will catch any omissions.

3. +++ b/core/modules/toolbar/tests/src/Nightwatch/Tests/toolbarTest.js
   @@ -0,0 +1,381 @@
   +    browser.drupalRelativeURL('/user');
   +    browser.drupalRelativeURL('/admin');
   

   Is there a specific reason for visiting to /user then /admin immediately after?

4. +++ b/core/modules/toolbar/tests/src/Nightwatch/Tests/toolbarTest.js
   @@ -0,0 +1,381 @@
   +          '#toolbar-item-administration-tray .toolbar-toggle-orientation button',
   

   This class is added via JavaScript in ToolbarVisualView, it seems like there's a risk of the test running before this element is rendered. Even it's unlikely, it can be protected against by running a browser.waitForElementPresent() for that selector before browser.executeAsync() is called.

5. +++ b/core/modules/toolbar/tests/src/Nightwatch/Tests/toolbarTest.js
   @@ -0,0 +1,381 @@
   +        const toolbar = document.querySelector('#toolbar-administration');
   +        const nextElement = toolbar.nextElementSibling.getBoundingClientRect();
   +        const tray = document
   +          .querySelector('#toolbar-item-administration-tray')
   +          .getBoundingClientRect();
   +        // Page content should start after the toolbar height to not overlap.
   +        return nextElement.top > tray.top + tray.height;
   

   This scenario where the overlap is a concern is when the toolbar tray is horizontal to be useful. Lets add a check for that so if this fails as part of a future change the feedback is more useful.

6. +++ b/core/scripts/dev/commit-code-check.sh
   @@ -353,7 +353,7 @@
   +  if [[ -f "$TOP_LEVEL/$FILE" ]] && [[ $FILE =~ \.js$ ]] && [[ ! $FILE =~ ^core/tests/Drupal/Nightwatch ]] && [[ ! $FILE =~ /tests/src/Nightwatch/ ]] && [[ ! $FILE =~ ^core/assets/vendor/jquery.ui/ui ]] && [[ ! $FILE =~ ^core/modules/ckeditor5/js/ckeditor5_plugins ]]; then
   

   Ah right, we've never had Nightwatch tests in a module. This is a good place for them!

Checked that all feedback from #11 was addressed. Great catches @bnjmnm!

Committed to 10.0.x and cherry picked to 9.4.x and 9.3.x as this is noninvasive as it only introduces test coverage that verifies the status quo.

This should make toolbar related issues much easier to work on, as there's now an easy way to know if a change caused unexpected results elsewhere in toolbar's logic.

The one change outside of adding these tests is to commit-code-check.sh, which correctly adds in-module Nightwatch tests to the js files that don't need to be checked for .es6.js equivalents.

Unfortunately this introduced a random fail (probably a race condition) that is happening at a very high rate. See for example:

• https://dispatcher.drupalci.org/job/drupal8_core_regression_tests/49403/...
• https://dispatcher.drupalci.org/job/drupal_patches/113982/console

So, I've reverted it from all three branches. When we debug similar race conditions in functional JS tests, we often make the random fail happen consistently by adding a sleep or such to prove there is a race condition, and the fix is usually to change to functionality that waits on the element to be available to click. Hopefully Nightwatch supports similar functionality.

The solution for the random failure looks good for me! Thank you for confirming that this actually resolves the random failure.

Would be good to have change record for this since it's a new API. I opened follow-up for using this new API in other tests: #3264940: Add optional parameter to Nightwatch drupalInstallModule command to enable module dependencies.

Impressive work by @nod_ on tracking down the race condition causing intermittent failures on the prior commit. Nightwatch isn't great with modules that have dependencies, and this significantly improved DX.

The commits just above & below #29 run the toolbar tests 100 consecutive times without a failure.

Committed to 10.0.x/9.4.x and backported to 9.3.x since this it is non-disruptive and the tests are confirming the status quo.

Nice work!