Extend the CKEditorIntegrationTest for DrupalImage

Thanks, this is a super helpful contribution! ❤️

+++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/CKEditorDrupalImageIntegrationTest.php
@@ -0,0 +1,170 @@
+class CKEditorDrupalImageIntegrationTest extends JavascriptTestBase {

Rather than creating an entirely new test class with a single testSomething() method, can you instead add a new test method to \Drupal\Tests\ckeditor\FunctionalJavascript\CKEditorIntegrationTest?

That will also make this issue much easier to review :)

Let's let testbot test #6 :)

1. +++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
   @@ -38,7 +39,7 @@ protected function setUp() {
   -      'name' => 'Filtered HTML',
   +      'name'   => 'Filtered HTML',
   

   Unnecessary change, let's revert this.

2. +++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
   @@ -118,4 +119,51 @@ public function testFragmentLink() {
   +    $this->clickOnElement('css', '.cke_button__drupalimage');
   ...
   +  protected function clickOnElement($selector_type, $selector) {
   

   Why can't this use \Drupal\Tests\BrowserTestBase::click()? Isn't the problem that you're not waiting for CKEditor to have loaded? (See \Drupal\Tests\ckeditor\FunctionalJavascript\AjaxCssTest::waitOnCkeditorInstance().)

Great! Look how wonderfully small the patch now became :) This is now ready, only the comments need to be refined a little bit more:

1. +++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
   @@ -118,4 +118,24 @@ public function testFragmentLink() {
   +   * Tests CKEditor button image apprear, works & dialog open.
   

   Tests if the Image button appears and works as expected.

2. +++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
   @@ -118,4 +118,24 @@ public function testFragmentLink() {
   +    // Asserts the Dialog open when clicking on the button.
   

   Asserts the image dialog opens when clicking the Image button.

Once those are done, this is RTBC!

Thanks! 👍

👍

FYI: upload interdiffs as .txt files, then test bot doesn’t try to test them! That’s the convention we’ve been using here on d.o for years :)

Just triggering an 8.4.x run as this is adding test coverage and it'd be great if it is backportable.

+++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
@@ -118,4 +118,25 @@ public function testFragmentLink() {
+    $web_assert->assertWaitOnAjaxRequest();
+
+    $web_assert->elementExists('css', '.ui-dialog');

These two calls could be replaced by $web_assert->waitForElement('css', '.ui-dialog'); but that's not necessary for the patch to be committed.

Crediting @Wim Leers for the code reviews.

Committed and pushed e9c9ee5282 to 8.5.x and 1136b5980d to 8.4.x. Thanks!

I believe this issue introduced a new random fail:
https://www.drupal.org/pift-ci-job/794678

https://www.drupal.org/pift-ci-job/794008 Edit: not that one, that one is different. See also #2701393: Switching between editors on the format configuration causes errors upon save.

This might be fixed by making the change mentioned in #20.

#2701393: Switching between editors on the format configuration causes errors upon save was reverted as well; note that these are two separate random failures in two separate tests, but they have a similar drawback with waiting for the AJAX request but not the element.

+++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
@@ -118,4 +118,25 @@ public function testFragmentLink() {
+    $web_assert->assertWaitOnAjaxRequest();
+
+    $web_assert->elementExists('css', '.ui-dialog');

In Settings Tray we found that relying on \Drupal\FunctionalJavascriptTests\JSWebAssert::assertWaitOnAjaxRequest can lead to random failures because often the following elementExists() check is passing just because the testbot is running fast enough.

So we added \Drupal\Tests\settings_tray\FunctionalJavascript\SettingsTrayJavascriptTestBase::assertElementVisibleAfterWait

which is just
$this->assertNotEmpty($this->assertSession()->waitForElementVisible($selector, $locator, $timeout));

So I am not sure if in this test your element will be visible but you could replace both these lines with

$this->assertNotEmpty($this->assertSession()->waitForElement('css', '.ui-dialog'));

Then you aren't waiting for ajax calls to be done which seems tricky but waiting for the effect of the ajax call, i.e. the element on the page.

@wengerk thanks for the patch.

Now let's see if we can make the this random failure happen by running only this test 110 times with and without fix in #30

darn you "\"

Second time lucky!

Committed and pushed cab7a12733 to 8.7.x and 4390dc6b92 to 8.6.x. Thanks!

YAY! This helps us be more confident about CKEditor updates :)