Problem/Motivation

Since the update of the Drupal-CI ChromeDriver we get our (un)fair share of test failures in (amongst others) \Drupal\Tests\quickedit\FunctionalJavascript\QuickEditFileTest.

Possible root cause

As can be seen in the test results of the QuickEditImageTest-Extra-CLUNK-Assertions.patch here: https://www.drupal.org/pift-ci-job/2382803, when the assertion $this->assertJsCondition("Drupal.quickedit.collections.entities.get('node/1[0]').get('state') === 'closed'"); fails,the field states are

1) Drupal\Tests\quickedit\FunctionalJavascript\QuickEditFileTest::testRemove
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    'node/1/body/en/full' => 'closed'
-    'node/1/title/en/full' => 'closed'
+    'node/1/body/en/full' => 'highlighted'
+    'node/1/title/en/full' => 'inactive'
 )

During the saving of a QuickEdited field (in this case the file field which is removed with the save), it _looks_ (at least on my local machine) like all other QuickEditable fields are all quickly queried for changes as well, making them highlighted and checked for a (very) brief moment in time, after which the become un-highlighted again.
In case of our test-failure, the body field remains highlighted, which matches with a screenshot from that moment (here's one that I prepared earlier)

Now here's where it all becomes very "I _think_ this happens":

The Save button of the "field-to-be-saved" (in our case the file field) currently overlaps another QuickEditable field (in this case body).
When clicking the Save button "somehow somewhere" there's a (frequent) case that re-highlights the body field during the save procedure, thus preventing the whole entity from getting field state closed.

Here's a screenshot (again prepare earlier) of Save button of the file-field overlapping the body field: Screenshot. As can be seen the Save button is right in the middle of the blue outlined QuickEditable zone of the body field.

As said, the above is all highly "Have no clue how it works, but at least to me this is a case that makes sense".
With that comes that it might very well be very untrue.

However the proposed resolution below _does_ work (at least gives no failure in the 1500x test runs, whilst the 1500x run has the usual failure rate).

Furthermore, I think the solution will/could/might work on a lot of our other (randomly) failing QuickEdit tests.

Proposed resolution

Move the "field-to-be-tested" to a place where it's QuickEdit Toolbar does _not_ overlap any other QuickEditable field, to prevent the pointer to be (accidentally) place on a re-highlight sweet-spot.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3280614

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Spokje created an issue. See original summary.

Spokje’s picture

Currently the \Drupal\Tests\quickedit\FunctionalJavascript\QuickEditFileTest fails _a lot_.

Attached patch tries to show why, by adding some extra assertions on field states and a screenshot, _if_ the original $this->assertJsCondition("Drupal.quickedit.collections.entities.get('node/1[0]').get('state') === 'closed'"); fails.

Patch runs the mentioned test (and _only_ that) 1500x.

Spokje’s picture

A 1500x \Drupal\Tests\quickedit\FunctionalJavascript\QuickEditFileTest as-is patch for baseline purposes and a patch with the proposed fix (more on that later in the IS), also running 1500x

Spokje’s picture

Issue summary: View changes
Spokje’s picture

Spokje’s picture

Issue summary: View changes
FileSize
37.66 KB
Spokje’s picture

Issue summary: View changes
Spokje’s picture

The actual patch with the proposed solution.

Spokje’s picture

Assigned: Spokje » Unassigned
Status: Active » Needs review
alexpott’s picture

Status: Needs review » Reviewed & tested by the community

Nice sleuthing. No harm in merging that. @Spokje++

catch’s picture

Version: 10.0.x-dev » 9.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 10.0.x and cherry-picked to 9.5.x/9.4.x/9.3.x, thanks!

  • catch committed a1c4ca7 on 10.0.x
    Issue #3280614 by Spokje: (Not so) Random test failures...
  • catch committed ce6cc9b on 9.3.x
    Issue #3280614 by Spokje: (Not so) Random test failures...
  • catch committed 3e59e19 on 9.4.x
    Issue #3280614 by Spokje: (Not so) Random test failures...
  • catch committed 6d19feb on 9.5.x
    Issue #3280614 by Spokje: (Not so) Random test failures...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.