[random test failure] Un-skip and fix QuickEditIntegrationTest::testArticleNode()

What we currently are doing around the point the test fails seems silly to me:

    // Wait for the saving of the tags field to complete.
    $this->assertJsCondition("Drupal.quickedit.collections.entities.get('node/1[0]').get('state') === 'closed'");
    $this->assertEntityInstanceStates([
      'node/1[0]' => 'closed',
    ]);

Wait for "state === 'closed'" and then assert that "state === 'closed'"?

I've had some success with the "fix" in the attached patch on a small scale.
Let's see how the fail rate of my approach (hopefully 0), compares to the baseline when we run both 1500x.

Unsure why this is against 9.3.x-dev, to prevent massive changing of platforms for testing temporarily moving this against 9.4.x-dev

to prevent massive changing of platforms for testing temporarily moving this against 9.4.x-dev

Meh, that didn't matter at all...

Moving to 10.0.x-dev where it should be IMHO

If/When this lands into Core, we also have to commit this to the Contrib incarnation of quickedit.

How about we do _not_ skip the test in the baseline patch... :/

So, AFAICT:

- Saving a node through quickedit in a Test,
- When TestBot is really busy,

takes longer than expected, about 1/2 times every 1500 runs.

Let's see if upping the time-out in the assert where it all falls down helps. If not, I'm out of (my very limited) options.

Upping the time-out in the assert doesn't seem to do much.

So, what does the page look like when things go wrong?

See attached screenshot on failing assert

So...

What's roughly going on AFAICT, is that the screenshot in #21 shows that sometimes the "QuickEdit Header" on the body-field stays open.
Everything else is doing it's stuff as it should, but the header means that the EntityInstanceState of node/1[0] remains forever in the 'closing' state, and thus preventing the

 $this->assertEntityInstanceStates([
        'node/1[0]' => 'closed',
      ]);

to pass.

I really can't see where this is coming from, our code, backbone or underscore.

I see several options to go from here:

1) We accept that, on heavy load, this tests fails randomly sometimes.
2) We skip the test.
3) We fix the test
4) Something in the middle of the above.

Currently we're doing 2), not a big fan personally.
We tried 1), but with the new ChromeDriver the fails happen too often.
Of course, we _want_ 3), but seeing that we need JS expertise, which is probably currently all needed for the upcoming beta-deadline. Also the fact that quickedit will leave Core and currently has no maintainer doesn't bode well for this option.

The current MR and #22 is my version of 4): We wrap the (sometimes) failing asserts for "waiting on closed-state" and testing for that state in a try, then have an empty catch and wrap the last assertions, which, as shown by #22 are still valid, in a finally-block.

This way we have no (semi-random) failures and still test that the main functionality of quickedit works without data-loss.
Of course we need a follow-up (and a @todo referring to it) to actually fix the issue someday, be it either here or in the Contrib incarnation of quickedit.

Changing to Needs review for more eyes/thoughts on this.

Sure looks like QuickEditFileTest has the same problem.

Before applying the same method to that one as well, I think we should agree on this method (catching the AssertionFailedError and basically hiding it) is an approach that we're OK with, until we (ever) come up with a real fix.

So I would suggest before spending time on trying to fix/hide the test-failures in QuickEditFileTest we try to land this one first, if that happens we turn the spotlight on QuickEditFileTest.

#14: That's because the first line has a timeout. The second one does not. IOW: the first tries to wait to reach that point, the second then asserts that we actually reached that state. I agree it's not pretty — but this was the best we could with the JS test infra do at the time.

#19 + #20: thanks for ruling that out as a possibility!

It sure is starting to look like this was caused by the recent chromedriver update on DrupalCI? Or maybe that merely exposed this perhaps long-existing problem?

#24:

1. See \Drupal\Tests\quickedit\FunctionalJavascript\QuickEditJavascriptTestBase::assertEntityInstanceStates() to know where that's coming from. Perhaps I'm misunderstanding your Q? It's in a Backbone collection that is controlled by Quick Edit's JS.
2. The fact that the "Quick Edit header" (official name: Entity Toolbar — see Drupal.quickedit.EntityToolbarView) stays open even after saving is a bug. So either our test infra is more brittle than before or it's a bug in chromedriver or it's a race condition in Quick Edit's JS that didn't happen previously.
3. Your 4 options make sense, but there is one that you haven't listed: increase the wait time. Can we bump it to something ridiculous like 100 seconds, and see if that causes tests to pass 100% of the time? 🤓🤓🤓🙈🙈🙈

Time to try out the approach/solution we've found in #3280614: (Not so) Random test failures QuickEditFileTest, which in TLDR; is: Move the field that is QuickEdit-ed the last to the top of the displayed fields.
(TooShortWantToRead; https://www.drupal.org/project/drupal/issues/3280614#summary-possible-root-cause).

First up a patch that asserts the field states if the $this->assertJsCondition("Drupal.quickedit.collections.entities.get('node/1[0]').get('state') === 'closed'"); fails.
It runs \Drupal\Tests\quickedit\FunctionalJavascript\QuickEditIntegrationTest::testArticleNode() (and only that test) 1500 times.

Looking at the screenshot I've attached earlier, we seem to have the same situation as #3280614: (Not so) Random test failures QuickEditFileTest , with the body being highlighted after a save on another field. If this is indeed the same we expect the following states after the above failure:

    'node/1/body/en/full' => 'highlighted'
    'node/1/title/en/full' => 'inactive'
    'node/1/field_tags/en/full' => 'inactive'

Grmbl, seems like the failure rate isn't that high (any more?)
Let's see if we can get a fail with a run of 2500 times.

So, we (or at least I) learned 2 things here:

1) The failure rate is pretty low at this point in time.
2) We indeed match the expectation that the body field is in a highlighted state:

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

Looking at the screenshot I've uploaded we also match the criteria of #3280614: (Not so) Random test failures QuickEditFileTest with having the save button of the latest field to be QuickEdited (in this case field_tags) overlapping another QuickEditable field (in this case body). (Screenshot)

Now let's try a 2500x run of \Drupal\Tests\quickedit\FunctionalJavascript\QuickEditIntegrationTest::testArticleNode() without any changes and at the same time a 2500x run of the same test but with the field_tags moved above the body field.

Also, in the moving-field_tags patch I've removed the

    $this->assertEntityInstanceStates([
      'node/1[0]' => 'closed',
    ]);

bit since, as mentioned in #14 the assertion above it ($this->assertJsCondition("Drupal.quickedit.collections.entities.get('node/1[0]').get('state') === 'closed'");), already assures this is the case, and would fail otherwise.

Ok, fix seems to work, let's get a decent patch up.

+++ b/core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditIntegrationTest.php
@@ -261,9 +271,6 @@ public function testArticleNode() {
-    $this->assertEntityInstanceStates([
-      'node/1[0]' => 'closed',
-    ]);

I understand your reasoning in #17. The intent of the assertJsCondition is to wait.

The intent of the assertion you removed is to show the evolution of this state throughout the phases of the test, using a consistent pattern. Removing this assertion breaks that consistent pattern.

So could you please revert this change? 🙏 Thanks 😊

Once that is reverted, this is RTBC IMO! 🥳

P.S.: nit:

+++ b/core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditIntegrationTest.php
@@ -138,6 +137,17 @@ public function testArticleNode() {
+    // Move "tags" field to the top of all fields, so its QuickEdit Toolbar
+    // won't overlap any QuickEdit-able fields, which causes (semi-)random test

s/QuickEdit/Quick Edit/

Tagging blocker because this fix is needed for #3267258: Remove Quick Edit support from editor.module.

The intent of the assertion you removed is to show the evolution of this state throughout the phases of the test, using a consistent pattern. Removing this assertion breaks that consistent pattern.

Waiting for condition === foo and then testing if condition === foo seems "odd" to me, but hey, if it brings this to RTBC, who am I to disagree (,I travelled the world and the seven seas, everybody's looking for something...)

… it also keeps the changes focused to the essence: make tests pass, don't change them.

Sorry, and … thank you 😊

The filename was changed in #3264633: Remove \Drupal\layout_builder\QuickEditIntegration and refactor it so that quickedit contrib provides the integration with layout builder but I went ahead and renamed it in the patch too and it applies cleanly with that to 10.0.x and 9.5.x, then the original patch still applies against 9.4 - committed/pushed to all three branches, thanks!

Tips hat @ catch

@catch: Looks like you never pushed a commit for this to 9.5.x, which is complicating progress at #3267258: Remove Quick Edit support from editor.module

Sorry, misread. See #44. ;) It's there.

Removed the "Needs followup"-tag, since there are numerous issues fixing all test-failures in the contrib reincarnation of Quick Edit, so this one is handled in those.