[random test failure] Restore and fix LayoutBuilderDisableInteractionsTest::testFormsLinksDisabled()

Nice, thanks @phenaproxima. I think we have a winner.

We should re-run that with the baseline, but #10's results look pretty solid.

Adding credit for Ben per #10.

Baseline patch and requeuing it with the above during the higher random fail window that seems to happen at night.

So we have a baseline test failing (#14) and running at the same time, a non-failing fix patch (#10).

Also we have a PDGE(=Pretty Damn Good Explanation) on _why_ the fix patch works in (#10).

I've attached a patch from #10 _without_ the changes in core/drupalci.yml, so only containing the fix.

I'm unsure against which Drupal Core version this patch should be against, according to here 9.3.x-dev seems the way to go and then port upwards.
Personally I would aim at 10.0.x-dev for downwards porting.

Let's just keep the middle ground that is 9.4.x-dev and conveniently is already the version of this issue.

Seeing that this is a _random_ failure that occurs (seemingly) only when TestBot is really busy, this patch is very likely to NOT-fail.
There's no proof in that to conclude it is fixing the issue.
(At least for me) The proof is in the test runs mentioned above.

Sense = made in #17.

Added new patch with comment.

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderDisableInteractionsTest.php
@@ -219,6 +218,12 @@ protected function assertContextualLinksClickable(): void {
+    $this->assertNotEmpty($assert_session->waitForElementVisible('css', '#drupal-off-canvas'));

I'm concerned this won't be sufficient. The successful fix in #3268368: Robustify and restore \Drupal\Tests\ckeditor5\FunctionalJavascript\MediaLibraryTest::testButton accomplished this by wating for something more specific than the dialog wrapper ID. For example, if you use debugger; to pause execution at Drupal.offCanvasafterCreate.afterCreate, #drupal-off-canvas already exists, but the dialog hasn't been fully resized/repositioned (and interestingly, the close button is not yet rendered).

Based on just comparing the markup pre and post resize, the following selector *might* be a reliable way to check for something that appears after resize + draggable completes #drupal-off-canvas > form[style=""]

FWIW #10 did look pretty sufficient based on the test results at that time, or at least mitigating.

1. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderDisableInteractionsTest.php
   @@ -31,6 +31,7 @@ class LayoutBuilderDisableInteractionsTest extends WebDriverTestBase {
   +    'off_canvas_test',
   
   @@ -219,6 +219,12 @@ protected function assertContextualLinksClickable(): void {
   +    $this->assertNotEmpty($assert_session->waitForElementVisible('css', '[data-resize-done="true"]'));
   

   Is the idea that we would use this test module and assertion attribute in more tests that look for things that might be in the off-canvas area?

   If so, should we provide an explicit trait and method for it?

2. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderDisableInteractionsTest.php
   @@ -219,6 +219,12 @@ protected function assertContextualLinksClickable(): void {
   +    // @see: https://www.drupal.org/project/drupal/issues/3268680
   

   We don't need to @see the issue that added the comment; that's what git blame is for. ;) The inline comment itself (aside from this line) is sufficient for that. :)

Meanwhile, I'll queue up some test result sets for the lastest patch and baseline.

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderDisableInteractionsTest.php
@@ -219,6 +219,12 @@ protected function assertContextualLinksClickable(): void {
+    // to become visible. This is to prevent a regular occurring random

Nit: should be "regularly occurring".

Addressed point number 2 of comment #23 and comment #24.

Missed the interdiff in comment #25, so adding here.

1. The "FAIL" patch is missing marking the test unskipped. ;) So of course it passes.
    
2. This is failing specifically on the new assertion, 100% of the time. Maybe we need to pass the assertion session into the method or something, because it's not defined in the scope inside the method? But no, your patch fixed that already. At least it's a 100% fail so can be debugged locally. Is it because it's creating a new instance of it? Or... no idea.
   

   Testing Drupal\Tests\layout_builder\FunctionalJavascript\LayoutBuilderDisableInteractionsTest
   F                                                                   1 / 1 (100%)
   
   Time: 01:34.400, Memory: 6.00 MB
   
   There was 1 failure:
   
   1) Drupal\Tests\layout_builder\FunctionalJavascript\LayoutBuilderDisableInteractionsTest::testFormsLinksDisabled
   Failed asserting that a NULL is not empty.
   
   
   /var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:121
   /var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
   /var/www/html/core/modules/system/tests/src/Traits/OffCanvasTestTrait.php:20
   /var/www/html/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderDisableInteractionsTest.php:228
   /var/www/html/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderDisableInteractionsTest.php:132
   /var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:726

3. The patch by itself has CS errors on the JS file:

   Created 17 Mar 2022 at 15:01 CDT. phenaproxima posted patch. Updated 17 Mar 2022 at 15:09 CDT.
   
   Custom Commands Failed - View results on dispatcher
   
   --- Commands Executed ---
   core/scripts/dev/commit-code-check.sh --drupalci
   Return Code: 1
   --- Output ---
   CSpell: passed
   
   ----------------------------------------------------------------------------------------------------
   Checking core/modules/system/tests/modules/off_canvas_test/js/resize-helper.es6.js
   
   yarn run v1.22.17
   $ cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js --check --file /var/www/html/core/modules/system/tests/modules/off_canvas_test/js/resize-helper.es6.js
   [20:09:25] '/var/www/html/core/modules/system/tests/modules/off_canvas_test/js/resize-helper.es6.js' is being checked.
   Done in 0.74s.
   
   /var/www/html/core/modules/system/tests/modules/off_canvas_test/js/resize-helper.es6.js
      2:1  error  Delete `⏎··`                    prettier/prettier
      5:3  error  Delete `··`                     prettier/prettier
      6:1  error  Replace `·····` with `···`      prettier/prettier
      7:4  error  Delete `··`                     prettier/prettier
      8:1  error  Delete `··`                     prettier/prettier
      9:4  error  Delete `··`                     prettier/prettier
     10:1  error  Delete `··`                     prettier/prettier
     11:1  error  Replace `·····` with `···`      prettier/prettier
     12:4  error  Delete `··`                     prettier/prettier
     13:1  error  Delete `··`                     prettier/prettier
     14:5  error  Delete `····`                   prettier/prettier
     15:1  error  Delete `····`                   prettier/prettier
     16:1  error  Replace `········` with `····`  prettier/prettier
     17:1  error  Delete `····`                   prettier/prettier
     18:3  error  Replace `··}⏎` with `};`        prettier/prettier
   </li>
   
   ✖ 15 problems (15 errors, 0 warnings)
     15 errors and 0 warnings potentially fixable with the `--fix` option.

   Maybe that makes sense in context? if you run the check locally?

This should fix the JS errors.

7 fails in nearly 10,000 results for the current baseline (the last three envs are about 80% done) means the fail rate of the fail-patch at this time of day is .0007 (or 0.07%) and (we're not actually sure of that last significant digit, could be anything between 0.1 and 1). That means every test has a 0.9993 chance of passing, which in turn means that there's about a 1% chance that we wouldn't see a still-possible fail in the number of queued test runs.

So I will requeue the whole set for the passing patch. :) (Not the fix-only patch though; testing it more than 1 time but fewer than 5,000 doesn't help us here.) :)

Queued it for 9 more runs. That'll make 16K total. If the base fail rate is roughly 0.0007, the chances of us NOT getting a fail are then miniscule. (If it were as low as 0.0001, then there would still be a 20% chance of us missing the fail with the current baseline. Isn't statistics fun! However, we got two more fails near the end of the last batches, so I think we're on the right power of 10.)

If we compare this with #14, I think we've shown pretty clearly that this fail rate goes way up at night.

I'm convinced. :)

Committed 32bee19 and pushed to 10.0.x. Thanks!
Committed and pushed 0fdbe80815 to 9.4.x and 0a06ce704a to 9.3.x. Thanks!

Backported to 9.3.x due as this is only test changes.

Need to rebuild the JS on 10.0.x because of JS build tool updates...

diff --git a/core/modules/system/tests/modules/off_canvas_test/js/resize-helper.js b/core/modules/system/tests/modules/off_canvas_test/js/resize-helper.js
index ee36776565..77c613f3fa 100644
--- a/core/modules/system/tests/modules/off_canvas_test/js/resize-helper.js
+++ b/core/modules/system/tests/modules/off_canvas_test/js/resize-helper.js
@@ -6,10 +6,12 @@
 **/

 (function (_ref) {
-  var offCanvas = _ref.offCanvas;
-  var originalResetSize = offCanvas.resetSize;
+  let {
+    offCanvas
+  } = _ref;
+  const originalResetSize = offCanvas.resetSize;

-  offCanvas.resetSize = function (event) {
+  offCanvas.resetSize = event => {
     originalResetSize(event);
     event.data.$element.attr('data-resize-done', 'true');
   };

Perhaps it's vanity, but I updated issue credit to the correctly spelled version of me.