Comments

nlisgo created an issue. See original summary.

nlisgo’s picture

nlisgo’s picture

Assigned: nlisgo » Unassigned
Status: Active » Needs review
scuba_fly’s picture

Ran the test locally. Don't see any simpletest / WebTestBase on the new code so that's good.

Attached is a green OK result of running the test in my phpstorm.

+1

dawehner’s picture

+++ b/core/modules/ckeditor/tests/src/Functional/CKEditorAdminTest.php
@@ -65,9 +65,9 @@ public function testExistingFormat() {
     $this->assertTrue(count($options) === 2, 'The Text Editor select has two options.');
-    $this->assertTrue(((string) $options[0]) === 'None', 'Option 1 in the Text Editor select is "None".');
-    $this->assertTrue(((string) $options[1]) === 'CKEditor', 'Option 2 in the Text Editor select is "CKEditor".');
-    $this->assertTrue(((string) $options[0]['selected']) === 'selected', 'Option 1 ("None") is selected.');
+    $this->assertTrue($options[0]->getText() === 'None', 'Option 1 in the Text Editor select is "None".');
+    $this->assertTrue($options[1]->getText() === 'CKEditor', 'Option 2 in the Text Editor select is "CKEditor".');
+    $this->assertTrue($options[0]->getAttribute('selected') === 'selected', 'Option 1 ("None") is selected.');

@@ -233,9 +235,9 @@ public function testNewFormat() {
-    $this->assertTrue(((string) $options[0]) === 'None', 'Option 1 in the Text Editor select is "None".');
-    $this->assertTrue(((string) $options[1]) === 'CKEditor', 'Option 2 in the Text Editor select is "CKEditor".');
-    $this->assertTrue(((string) $options[0]['selected']) === 'selected', 'Option 1 ("None") is selected.');
+    $this->assertTrue($options[0]->getText() === 'None', 'Option 1 in the Text Editor select is "None".');
+    $this->assertTrue($options[1]->getText() === 'CKEditor', 'Option 2 in the Text Editor select is "CKEditor".');
+    $this->assertTrue($options[0]->getAttribute('selected') === 'selected', 'Option 1 ("None") is selected.');

You can use assertSame instead here

dawehner’s picture

Status: Needs review » Needs work
nlisgo’s picture

Status: Needs work » Needs review
FileSize
3.43 KB
10.4 KB

Addressed the feedback in #5.

Thanks @dawehner

Status: Needs review » Needs work

The last submitted patch, 7: convert_web_tests_to-2863996-7.patch, failed testing.

nlisgo’s picture

New patch incoming. Not sure why composer.lock changes were in my patch.

nlisgo’s picture

Status: Needs work » Needs review
FileSize
3.43 KB
8.59 KB

I created the diff in #7 from my local 8.4.x which was one commit behind HEAD and I had rebased my feature branches to origin/8.4.x! doh!

This patch should take!

dawehner’s picture

+++ b/core/modules/ckeditor/tests/src/Functional/CKEditorAdminTest.php
@@ -62,12 +62,12 @@ public function testExistingFormat() {
+    $this->assertSame(1, count($select), 'The Text Editor select exists.');
+    $this->assertSame(0, count($select_is_disabled), 'The Text Editor select is not disabled.');
+    $this->assertSame(2, count($options), 'The Text Editor select has two options.');

@@ -230,12 +232,12 @@ public function testNewFormat() {
+    $this->assertSame(1, count($select), 'The Text Editor select exists.');
+    $this->assertSame(0, count($select_is_disabled), 'The Text Editor select is not disabled.');
+    $this->assertSame(2, count($options), 'The Text Editor select has two options.');

Note: you can use assertCount here.

Jo Fitzgerald’s picture

Changed assertSame() to assertCount().

nlisgo’s picture

Changed assertSame() to assertCount() in CKEditorAdminTest::testNewFormat()

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you both!

Wim Leers’s picture

As a maintainer of the CKEditor module, I think this looks great :) Thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 40d1faf to 8.4.x and 36657a4 to 8.3.x. Thanks!

As a test-only change backported to 8.3.x

  • alexpott committed 40d1faf on 8.4.x
    Issue #2863996 by nlisgo, Jo Fitzgerald, scuba_fly, dawehner: Convert...

  • alexpott committed 36657a4 on 8.3.x
    Issue #2863996 by nlisgo, Jo Fitzgerald, scuba_fly, dawehner: Convert...

Status: Fixed » Closed (fixed)

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