[random test failure] Make QuickEditIntegrationTest more robust and fail proof

Thanks for doing this! 🙏

mostly failed but passed randomly.

😨

Out of 100 runs, this failed 54 times.

Attempting to apply the techniques from #2948828-136: Layout Builder's Field Blocks do not work with Quick Edit here.

This is probably fixed by #3082602: Remove transform rule from css_disable_transitions_test. Going to mark as duplicate. Worst case we can re-open it.

CSS transitions aren't disabled in 8.x, so while that issue might reduce the frequency, it probably doesn't actually fix this.

I have seen several fails in this test in the past few days. Example:
https://www.drupal.org/pift-ci-job/1777967

This is definitely still a problem.

Just got this in head, going to retest

I'm getting this one and two others in issues I follow fairly frequently. I think about once a week in total, and it's often QuickEditIntegrationTest... Could we maybe remove the 3 most common culprits (look through the many RTBC issues that have been hanging out for months in the RTBC queue, and see what they've failed on) and not put these repeat offenders back into Core until they will reliably work?

See also #3165263: Allow known flaky tests to be automatically repeated

This may be fixed by #3175112: hold_test module creates files in incorrect place leading to possible random errors.

Going to go ahead and mark this as duplicate, since that issue is the first concrete fix for this for a long time. If it does recur, we can re-open.

Unfortunately that didn't solve it, this just failed again in https://www.drupal.org/pift-ci-job/1848467

That is a shame, I think we should do this.

    // Wait for CKEditor to load, then verify it has.
    $this->assertJsCondition('CKEDITOR.status === "loaded"');

Only waits for Ckeditor to be ready in the background it doesn't wait for the instances or loaded event to fire - see https://github.com/ckeditor/ckeditor4/blob/d1710c198d8f5007e8d04fed9870b...

I think we can fix this by waiting for the what we expect rather than for ckeditor's internal state.

Been meaning to fix this the unhelpful reporting from WebDriverCurlService for a while.

For a while I've been wondering if concurrency might be causing an issue in JS tests.

And with a concurrency of 5

Add now 10

Let's see if going incognito helps...

Well as useful as the test passing all the time is the test failing all the time! So the check for

    $this->assertEntityInstanceFieldMarkup('node', 1, 0, [
      'node/1/title/en/full'      => '.quickedit-changed',
      'node/1/field_tags/en/full' => '.quickedit-changed',
    ]);

is bogus. Once "save" is pressed in quickedit this class is eventually removed. It passes now on HEAD because this happens after the AJAX response so most of the time these elements do indeed exist. I have no idea why concurrency makes it more likely to happen - maybe things just go quicker when everything is running the same code.

fingers-crossed this one will pass...

Let's run all the tests...

Consistency... hold_test_request() is never actually called so it doesn't matter but it is best to be consistent. Let's enforce the consistency and the relationship to the time waiting in HoldTestSubscriber

Seeming as the latest approach fixes the test

Oh fun. In #35 QuickEditImageTest - it's pressing save and expecting things to test able and not change. We need to use hold_test so we can test what is it testing. Also in reviewing this I realised we can re-instate the checks I removed ....

+++ b/core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditIntegrationTest.php
@@ -273,16 +271,18 @@ public function testArticleNode() {
-    $this->assertEntityInstanceFieldMarkup('node', 1, 0, [
-      'node/1/title/en/full'      => '.quickedit-changed',
-      'node/1/field_tags/en/full' => '.quickedit-changed',
-    ]);

but move them before the hold_test_response() is released.

+++ b/core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditJavascriptTestBase.php
@@ -255,25 +255,9 @@ function () {
   protected function assertEntityInstanceFieldMarkup($entity_type_id, $entity_id, $entity_instance_id, array $expected_field_attributes) {
-    $entity_page_id = $entity_type_id . '/' . $entity_id . '[' . $entity_instance_id . ']';
-    $expected_field_attributes_json = json_encode($expected_field_attributes);
-    $js_match_field_element_attributes = <<<JS
-function () {
-  var expectations = $expected_field_attributes_json;
-  var entityCollection = Drupal.quickedit.collections.entities;
-  var entityModel = entityCollection.get('$entity_page_id');
-  return entityModel.get('fields').reduce(function (result, fieldModel) {
-    var fieldID = fieldModel.get('fieldID');
-    var element = fieldModel.get('el');
-    var matches = element.webkitMatchesSelector(expectations[fieldID]);
-    result[fieldID] = matches ? matches : element.outerHTML;
-    return result;
-  }, {});
-}()
-JS;
-    $result = $this->getSession()->evaluateScript($js_match_field_element_attributes);
     foreach ($expected_field_attributes as $quickedit_field_id => $expectation) {

So I think we one remaining question here is should we remove the now unused arguments. The only thing that is used now the $expected_field_attributes. Maybe we should tidy that up in a follow-up.

Let's trigger a deprecation. Imo this is not worth a CR but if there are any custom tests using this then at least you can ignore deprecations to make them pass... or make the change. There's no contrib using this.

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderQuickEditTest.php
@@ -202,8 +202,12 @@ protected function assertEntityInstanceFieldStates($entity_type_id, $entity_id,
-  protected function assertEntityInstanceFieldMarkup($entity_type_id, $entity_id, $entity_instance_id, array $expected_field_attributes) {
...
+  protected function assertEntityInstanceFieldMarkup(array $expected_field_attributes) {
+    if (func_num_args() === 4) {
+      $expected_field_attributes = func_get_arg(3);
+      @trigger_error('Calling ' . __METHOD__ . ' with 4 arguments is deprecated in drupal:9.1.x and will throw an error in drupal:10.0.0.', E_USER_DEPRECATED);
+    }

I don't think the deprecation will ever trigger, as $entity_type_id is never going to be an array and so it will fail the type check first.

Created #3178758: Remove deprecation layer and add argument type to ::assertEntityInstanceFieldMarkup for the deprecation removal and adding the typehint.

This certainly looks like it should be more reliable, asserting in Mink instead of in-browser JS and not relying on CKEditor internal state definitely feels much cleaner.

Now that #3048495: Fix Drupal.Semantics.FunctionTriggerError coding standard is RTBC here's an updated patch to make the two new deprecation messages follow standards. Fixing them here has two benefits (a) the people involved here get to check the message in context, and (b) if this issue is committed first it avoids me re-rolling on the main issue.

I've set this back to NR for #47 but of course, if you don't want to fix the message, just put it to RTBC for patch #45

Didn't spot here that the deprecated method name should end in () - if you want to make that change and upload it feel free to mark it RTBC.

OK, thanks for that. Here's the updated patch and interdiff.

Committed/pushed to 9.2.x and cherry-picked back to 9.0.x

Doesn't cherry-pick to 8.9.x - marking fixed but please re-open with a backport patch if someone does one.