See #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase)

CommentFileSizeAuthor
#13 convert_web_tests_to-2863996-13.patch8.56 KBnlisgo
#13 interdiff-12-13.txt1.41 KBnlisgo
#12 2863996-12.patch8.57 KBjofitz
#12 interdiff-10-12.txt1.41 KBjofitz
#10 convert_web_tests_to-2863996-10.patch8.59 KBnlisgo
#10 interdiff-2863996-2-10.txt3.43 KBnlisgo
#7 convert_web_tests_to-2863996-7.patch10.4 KBnlisgo
#7 interdiff-2863996-2-7.txt3.43 KBnlisgo
#4 ImageFieldDefaultImagesTest_php_-_drupal-vm_-____sites_drupal-vm_.png217.18 KBscuba_fly
#2 convert_web_tests_to-2863996-2.patch7.63 KBnlisgo
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nlisgo created an issue. See original summary.

nlisgo’s picture

nlisgo CreditAttribution: nlisgo commented
FileSize
convert_web_tests_to-2863996-2.patch7.63 KB
nlisgo’s picture

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

scuba_fly
Location Netherlands, Vinkeveen
CreditAttribution: scuba_fly at LimoenGroen commented
FileSize
ImageFieldDefaultImagesTest_php_-_drupal-vm_-____sites_drupal-vm_.png217.18 KB

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

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented 
+++ 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

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented
Status: Needs review » Needs work
nlisgo’s picture

nlisgo CreditAttribution: nlisgo commented
Status: Needs work » Needs review
FileSize
interdiff-2863996-2-7.txt3.43 KB
convert_web_tests_to-2863996-7.patch10.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

nlisgo CreditAttribution: nlisgo commented

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

nlisgo’s picture

nlisgo CreditAttribution: nlisgo commented
Status: Needs work » Needs review
FileSize
interdiff-2863996-2-10.txt3.43 KB
convert_web_tests_to-2863996-10.patch8.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

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented 
+++ 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.

jofitz’s picture

jofitz CreditAttribution: jofitz at ComputerMinds commented
FileSize
interdiff-10-12.txt1.41 KB
2863996-12.patch8.57 KB

Changed assertSame() to assertCount().

nlisgo’s picture

nlisgo CreditAttribution: nlisgo commented
FileSize
interdiff-12-13.txt1.41 KB
convert_web_tests_to-2863996-13.patch8.56 KB

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

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

Thank you both!

Wim Leers’s picture

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

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

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
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.