Problem/Motivation

#3066447-53: [random test failure] Random failures building media library form after uploading image (WidgetUploadTest) points out that there's a random fail on \Drupal\Tests\ckeditor5\FunctionalJavascript\CKEditor5Test::testEditorFileReferenceIntegration with PostgreSQL. However, that test is failing on my local environment with PostgreSQL consistently.

Proposed resolution

Fix it.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#7 3250587-7.patch1.1 KBlauriii
#7 3250587-7-30x.patch2.83 KBlauriii
#3 3250587-2-fix-only.patch981 byteslauriii
#2 3250587-2-30x.patch2.69 KBlauriii
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lauriii created an issue. See original summary.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
FileSize
3250587-2-30x.patch2.69 KB
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Active » Needs review
FileSize
3250587-2-fix-only.patch981 bytes

Here's patch from #2 without the drupalci.yml changes.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Fascinating!

So your theory is … that the file upload did not properly finish? 🤔

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented

#4 that's right.

+++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/CKEditor5Test.php
@@ -382,6 +382,10 @@ public function testEditorFileReferenceIntegration() {
     $assert_session->assertWaitOnAjaxRequest();

We should probably remove this since the image upload is not a Drupal Ajax request meaning that this doesn't do what we expect it to do.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Priority: Major » Critical

Bumping to critical, this is rather consistently failing on 9.4.x HEAD for Postgres

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
FileSize
3250587-7-30x.patch2.83 KB
3250587-7.patch1.1 KB

This implements the change I proposed in #5.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Status: Needs review » Reviewed & tested by the community

This is a pretty unambiguous improvement. The test had been calling $assert_session->assertWaitOnAjaxRequest(); to wait for the completion of the image upload/render before advancing. However, that process is not handled by an AJAX request. assertWaitOnAjaxRequest() introduces a brief wait, long enough to be sufficient for tests to sometimes pass, particularly in faster environments. It's not surprising that this use of $assert_session->assertWaitOnAjaxRequest(); is causing intermittent failures.

By changing this to $this->assertNotEmpty($assert_session->waitForElement('css', '.ck-content img[data-entity-uuid]'));, the test is actually waiting for the upload/render to complete before advancing. Waiting for an element to appear/disappear is preferable to assertWaitOnAjaxRequest(); even in situations where Ajax requests are being used, so this is a welcome change, and one I'm glad @lauriii spotted.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

and one I'm glad @lauriii spotted.

+1000!

Wow.

What a find.

Image uploads in CKEditor 5 are handled slightly differently than in CKEditor 4; they're uploaded by the CKEditor 5 plugin itself, not via a clunky Drupal-owned, AJAX-powered EditorImageDialog. That's why assertWaitOnAjaxRequest() is not sufficient in this case. It went unnoticed all this time just because only PostgreSQL tests happen to be slower… 🤯

Epic find, @lauriii!

Agreed with RTBC.

  • catch committed 40a0e8a on 9.4.x 
    Issue #3250587 by lauriii, bnjmnm: \Drupal\Tests\ckeditor5\...

  • catch committed af165d2 on 9.3.x 
    Issue #3250587 by lauriii, bnjmnm: \Drupal\Tests\ckeditor5\...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 9.4.x-dev » 9.3.x-dev
Status: Reviewed & tested by the community » Fixed

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

Status: Fixed » Closed (fixed)

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

Grevil’s picture

Grevil CreditAttribution: Grevil at DROWL.de commented

Hey guys, I know this issue is closed, but I just stumbled upon it and was wondering how it is possible to run "waitForElement()" on $assert_session?

$assert_session is initialized with, $assert_session = $this->assertSession(); and "$this->assertSession()" will return a "\Drupal\Tests\WebAssert" Object.

But in https://api.drupal.org/api/drupal/core%21tests%21Drupal%21Tests%21WebAss..., there is not a single note about "waitForElement()". Am I missing something? Or is it just not documented yet?