Avoid random failures in JavascriptTestBase when testing functionality in a dialog

+++ b/core/tests/Drupal/FunctionalTests/AssertDialogTrait.php
@@ -0,0 +1,23 @@
+    $this->assertTrue(in_array('dialog_test', self::$modules), 'The dialog_test module is required for this assertion.');

This condition feels more like an assert() assertion

Tagging and add a credit message :P

I will review it this weekend or so if no one does it before me.

I tried to triage this and review, but I just don't know how to reproduce this. Added Needs issue summary update tag, hoping someone adds steps to reproduce, or maybe droplet comes back and reviews this.

@balagan That's good work so far on triage. I could give you credit if you document the rest of the triage steps (even if brief). It is the only way to know if the triage has actually been completed. Here are some made-up examples of documented triage steps:

• I tested the steps to reproduce and they did (or did not) work (so I am tagging it "Needs issue summary update").
• I searched for duplicate issues but could not find any.
• I checked the issue summary and it is accurate and up-to-date.
• Etc...

Thank you!

Patch looks good to me. Do we need core/modules/system/tests/modules/dialog_test/js/dialog.debug.js in for all the dailogs?

.

A few small comments nothing of any consequence,

1) Since this issue was started, core now exclusively uses es6.js files which are transpiled into js

2) The variable dependency injection of the file was incorrect

This is the change.

-})(jQuery, Drupal, drupalSettings);
+})(jQuery, window, Drupal);

This patch is blocking #2832672: [PP-1] Upgrade jcalderonzumba/* for better test performance so RTBCing it.

1. +++ b/core/modules/system/tests/modules/dialog_test/js/dialog.debug.es6.js
   @@ -0,0 +1,23 @@
   +(function ($, window, Drupal) {
   

   Use arrow functions.

2. +++ b/core/modules/system/tests/modules/dialog_test/js/dialog.debug.es6.js
   @@ -0,0 +1,23 @@
   +  'use strict';
   

   `use strict` not required here.

3. +++ b/core/modules/system/tests/modules/dialog_test/js/dialog.debug.es6.js
   @@ -0,0 +1,23 @@
   +  $('#drupal-modal').on('dialogContentResize',  () => {
   

   Too many spaces.

+++ b/core/modules/views_ui/tests/src/FunctionalJavascript/FilterCriteriaTest.php
--- /dev/null
+++ b/core/tests/Drupal/FunctionalTests/AssertDialogTrait.php

+++ b/core/tests/Drupal/FunctionalTests/AssertDialogTrait.php
@@ -0,0 +1,23 @@
+  protected function waitForDialog($timeout = 10000) {

This adds a method that seems to be standalone.

+++ b/core/modules/system/tests/modules/dialog_test/dialog_test.module
@@ -0,0 +1,8 @@
+  $page['#attached']['library'][] = 'dialog_test/dialog_debug';

+++ b/core/modules/system/tests/modules/dialog_test/js/dialog.debug.js
@@ -0,0 +1,23 @@
+    Drupal.dialog_debug.has_open_dialog = true;

+++ b/core/modules/views_ui/tests/src/FunctionalJavascript/FilterCriteriaTest.php
@@ -11,10 +12,12 @@
-  public static $modules = ['node', 'views', 'views_ui'];
+  public static $modules = ['node', 'views', 'views_ui', 'dialog_test'];

+++ b/core/tests/Drupal/FunctionalTests/AssertDialogTrait.php
@@ -0,0 +1,23 @@
+    $condition = 'Drupal.dialog_debug.has_open_dialog == true';

However, it will silently do nothing if the `dialog_test` module is not installed.

I think the name is misleading. It only works for opening, not closing.
Furthermore, it does not account for a dialog that uses a fade effect, which assertWaitOnAjaxRequest() does protect against.

I will spend sometime today resolving lint conflicts and will address the other issue while I am at it.

Fixed #26.

+++ b/core/modules/views_ui/tests/src/FunctionalJavascript/FilterCriteriaTest.php
@@ -11,10 +12,12 @@
+  public static $modules = ['node', 'views', 'views_ui', 'dialog_test'];

+++ b/core/tests/Drupal/FunctionalTests/AssertDialogTrait.php
@@ -0,0 +1,23 @@
+/**
+ * Provides methods for assertions in dialogs.
+ */
+trait AssertDialogTrait {

Can we add a test inside core, outside of the views module? This makes it for example less likely that we accidentally drop it.

reroll

Tagging so I can find this in the search; it's not a random test failure itself but it's about them.

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 200, following Review a patch or merge require as a guide.

# D10 version
D10 version needed
At this time we would need a D10.1.x patch or MR for this issue.

Rerolled for D10
Removed the es6 file from the test module.

The statusTest has changed some too so tried to address those.

Are there any other tests that can use this? I think this might have been what dawehner was getting at in #31.

I think a dedicated test for this trait/JS is what was asked for. Sounds like a good addition, currently if we were to remove the Views test, this functionality has no coverage.

Do you have an example of testing a trait?

Something like this?

Nice, that looks great.

Code looks good, we are only updating 1 test here, should we do more here of should we open a follow-up to change more tests to use this?
I think landing this first is good, so marking RTBC, not making the follow up yet till others agree that would be the right way to handle this.

Doing more in a follow-up seems good to me.

Ok, here is that follow up #3350260: Convert all JavaScript tests that use dialogs to use waitForDialog()

Random failure..

Since this introduces changes intended to reduce random fails, this should have patches that customize drupalci.yml to run a single random-fail prone test several hundred times (possibly as many times as the test runner time limit allows). Include a version with and without the changes introduced here to compare the failure rate.

I also noticed that the solution here resembles something already in core specific to off canvas. Most of the lifting is done in these files:
https://git.drupalcode.org/project/drupal/-/blob/10.1.x/core/modules/sys...
https://git.drupalcode.org/project/drupal/-/blob/10.1.x/core/modules/sys...

Perhaps the changes here can be integrated the off canvas ones, even if it's just having the traits in the same dir/namespace?

Since this introduces changes intended to reduce random fails, this should have patches that customize drupalci.yml to run a single random-fail prone test several hundred times (possibly as many times as the test runner time limit allows). Include a version with and without the changes introduced here to compare the failure rate.

Adding both those here, before we start work on (possible) integrating with the off canvas solution.

So what we've learned here is that FilterCriteriaTest is not broken (enough) to make any difference.

Not saying the approach in the patch for dialogs is wrong, just that it can't be tested with FilterCriteriaTest

MediaLibraryTest::testButton() is a recently skipped random failure that checks for a dialog, so that might be a good one to try. #3351596: Skip Drupal\Tests\ckeditor5\FunctionalJavascript\MediaLibraryTest

Sadly also this one doesn't seem broken enough to be used for any proof.

Maybe LayoutBuilderUiTest - just seen it come up:

.Drupal\Tests\layout_builder\FunctionalJavascript\LayoutBuilderUiTest
✗	
Drupal\Tests\layout_builder\FunctionalJavascript\LayoutBuilderUiTest
fail: [Other] Line 0 of sites/default/files/simpletest/phpunit-107.xml:
PHPUnit Test failed to complete; Error: PHPUnit 9.5.28 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\layout_builder\FunctionalJavascript\LayoutBuilderUiTest
F.S.                                                                4 / 4 (100%)

Time: 00:36.411, Memory: 4.00 MB

There was 1 failure:

1) Drupal\Tests\layout_builder\FunctionalJavascript\LayoutBuilderUiTest::testReloadWithNoSections
Error: cannot call methods on dialog prior to initialization; attempted to call method 'widget'
    at Function.error (http://php-apache-jenkins-drupal-

https://www.drupal.org/pift-ci-job/2634951

If we can't get it to fail out of 1500 runs, it suggests the recent instability of all these tests might not be related to the changes in this patch though.

If we can't get it to fail out of 1500 runs, it suggests the recent instability of all these tests might not be related to the changes in this patch though.

Even more: If we can't get MediaLibraryTest::testButton() to fail once in 1500 times when run on it's own, I fear it looks like the failure there has to do with concurrent running other tests?

Looks like it's failing pretty well :D

60-ish failures

got a potential fix for the issue in #65 over at #3350972: [random test failure] Drupal\Tests\layout_builder\FunctionalJavascript\LayoutBuilderUiTest::testReloadWithNoSections() which isn't quite an issue with the timing in tests.

Just fixing tag text.

Perhaps a lead on how to reproduce this reliably: smaller screen height resolution seems to affect it a lot.

We have a huge suite of behat (selenium chromium) tests on a project at work. I wanted to change the resolution (from 1600x1200 to 1600x900) and noticed a lot of failures with modals. Traced it to the debounce (20ms) in Drupal.dialog.resetSize causing dialogs to bounce 2-3 times, then stumbled on this issue. In my case, the modal appears, selenium2 webdriver attempts to scroll in the modal to click on a link inside, but the modal repositions again before it can scroll and selenium throws a "move target out of bounds" error because the links is out of reach, still hidden below. (what a nightmare to debug).
I simply reverted to 1600x1200 for now, the modal seems more stable, way less repositioning for some reason.

Does #3472624: Error: cannot call methods on dialog prior to initialization; attempted to call method 'option' solve this?
Specifically the change to execute debounce immediately/on leading edge in dialog.position.js:
const autoResize = debounce(resetSize, 20, true);