Problem/Motivation

As discoverd in #2832672: Upgrade jcalderonzumba/mink-phantomjs-driver for better test performance a test opening a dialog and then peforming actions within the opened dialog are at risk for random failure.

The random failure is caused by a dialog resize/reposition after content has been inserted. Currently there is no way to wait for this as this behavior is debounced.

Currently the random failures do not occur because the Mink Phantomjs driver waits an additional 100ms on each wait statement. However on upgrading the driver or switching to another driver (#2775653: [PP-2] JavascriptTests with webDriver) this problem will become relevant.

Proposed resolution

Either:
1. Provide a better way to assert the dialog has opened.
2. Remove the random failure factor for opening a dialog.

Steps to reproduce:

This issue improves the way we wait for a dialog to open. The reason is random failure and therefore difficult to reproduce.

To reproduce the random fail do the following:
- Apply the patch from #2832672: Upgrade jcalderonzumba/mink-phantomjs-driver for better test performance, this will make the random fail occur more often.
- Do not apply the patch from this issue.
- Run the test in views_ui/tests/src/FunctionalJavascript/FilterCriteriaTest.php multiple times untill the test fails.
- See the excellent research in #2832672: Upgrade jcalderonzumba/mink-phantomjs-driver for better test performance by @tacituseu to see an explaination on the fails.

I have run the tests on multiple 25x runs locally and I got the fails frequently before applying this patch and no more after applying this patch.

Thanks @michielnugter for the patch. And tacituseu & droplet did a lot of researching on #2832672: Upgrade jcalderonzumba/mink-phantomjs-driver for better test performance. Please credit them also if we can.

Comments

michielnugter created an issue. See original summary.

michielnugter’s picture

Attached a convert from the original patch and used a trait, this is a fix using option 1. Not very happy yet with the way the dialog_test module is installed and checked on, don't know a better way yet.

Alternatives:
- Wait for #2844582: Move inline javascript in JSWebAssert into a separate javascript file to get in and move the assertion to JSWebAssert.

Questions:
- I kept the updated FilterDialogTest as a test for the change. Should this be split of later on?
- Is a separate test of the dialog required or does the FilterDialogTest suffice?

michielnugter’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 2: 2856047-2-25x.patch, failed testing.

michielnugter’s picture

Status: Needs work » Needs review
FileSize
5.66 KB
6.25 KB

asserT instead of asser

Status: Needs review » Needs work

The last submitted patch, 5: 2856047-5-25x.patch, failed testing.

michielnugter’s picture

Status: Needs work » Needs review
FileSize
5.66 KB

self::$modules instead of $this->modules.

dawehner’s picture

+++ 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

michielnugter’s picture

I converted it to a assertArrayHasKey(), the assert() method really is deprecated right?

Some other clean-ups as well.

Status: Needs review » Needs work

The last submitted patch, 9: 2856047-9.patch, failed testing.

michielnugter’s picture

Status: Needs work » Needs review
FileSize
784 bytes
5.45 KB

Right, wrong kind of assert.

I was thinking about this a little more and the current check can fail if the module is loaded on a base class because of the way the $modules array works. I therefore removed the check and added a hint in the phpdoc for this method. Is this acceptable?

droplet’s picture

Issue summary: View changes
Issue tags: +JavaScript

Tagging and add a credit message :P

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

droplet’s picture

Issue summary: View changes
michielnugter’s picture

Issue summary: View changes

Thanks in advance @droplet. And very true on the credits for you and tacituseu, a lot of preliminary research went into the solution for this one.

balagan’s picture

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’s picture

Issue summary: View changes
michielnugter’s picture

Issue summary: View changes
michielnugter’s picture

I updated the steps to reproduce.

cilefen’s picture

@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!