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
Comment | File | Size | Author |
---|---|---|---|
#8 | 3280614-8.patch | 1.27 KB | Spokje |
| |||
#6 | screen_1652505331_PRE.png | 37.66 KB | Spokje |
#5 | screen_QuickEditFileTest_CLUNK_1652505342.png | 25.37 KB | Spokje |
#3 | QuickEditImageTest-fix-1500x.patch | 3.01 KB | Spokje |
| |||
#3 | QuickEditImageTest-baseline-1500x.patch | 1.74 KB | Spokje |
Issue fork drupal-3280614
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:
Comments
Comment #2
SpokjeCurrently 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.
Comment #3
SpokjeA 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 1500xComment #4
SpokjeComment #5
SpokjeComment #6
SpokjeComment #7
SpokjeComment #8
SpokjeThe actual patch with the proposed solution.
Comment #9
SpokjeComment #10
alexpottNice sleuthing. No harm in merging that. @Spokje++
Comment #11
catchCommitted/pushed to 10.0.x and cherry-picked to 9.5.x/9.4.x/9.3.x, thanks!