Follow-up to #2864005: Convert web tests to browser tests for block_content module
Convert \Drupal\block_content\Tests\BlockContentTypeTest to BTB

core/modules/block_content/src/Tests/BlockContentUpdateTest.php is getting converted in #2905627: Part-2: Convert UpdatePathTestBase to BrowserTestBase

CommentFileSizeAuthor
#11 interdiff-8-11.txt1.22 KBAnonymous (not verified)
#11 2887311-11.patch2.25 KBAnonymous (not verified)
#8 2887311-8.patch1.81 KBnaveenvalecha
#7 2887311-6.patch923 bytesnaveenvalecha

Comments

naveenvalecha created an issue. See original summary.

naveenvalecha’s picture

naveenvalecha
He/Him
commented
Issue tags: -DevDaysSeville

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

naveenvalecha’s picture

naveenvalecha
He/Him
commented
Issue summary: View changes
Status: Postponed » Active

Blocker has landed

naveenvalecha’s picture

naveenvalecha
He/Him
commented
Title: [PP-1] Convert web tests to browser tests for block_content module Part -2 » Convert web tests to browser tests for block_content module Part -2

Updated title

naveenvalecha’s picture

naveenvalecha
He/Him
commented
Title: Convert web tests to browser tests for block_content module Part -2 » Convert BlockContentTypeTest web tests to browser tests for block_content module
Issue summary: View changes
Status: Active » Needs review
Related issues: +#2335057: Undefined method getUrl in AssertContentTrait::parse causes fatal error

Here's the start with failing tests

naveenvalecha’s picture

naveenvalecha
He/Him
commented
StatusFileSize
new 2887311-6.patch923 bytes
naveenvalecha’s picture

naveenvalecha
He/Him
commented
StatusFileSize
new 2887311-8.patch1.81 KB

This would be green.

+++ b/core/modules/block_content/tests/src/Functional/BlockContentTypeTest.php
@@ -137,7 +137,7 @@ public function testBlockContentTypeEditing() {
+    $this->assertFalse($this->cssSelect('tr#block-body'), 'Body field was not found.');

I'm to make this change because assertNoRaw is also getting the body tag in the html and it was able to find the body tag. So I replaced it with the cssSelect. Is this #2335057: Undefined method getUrl in AssertContentTrait::parse causes fatal error similar?

$ ../vendor/bin/phpunit modules/block_content/tests/src/Functional/BlockContentTypeTest.php 
PHPUnit 4.8.35 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\block_content\Functional\BlockContentTypeTest
.E..

Time: 2.98 minutes, Memory: 8.00MB

There was 1 error:

1) Drupal\Tests\block_content\Functional\BlockContentTypeTest::testBlockContentTypeEditing
Behat\Mink\Exception\ExpectationException: The string "Body" appears in the HTML response of this page, but it should not.

/Applications/MAMP/htdocs/d8/vendor/behat/mink/src/WebAssert.php:770
/Applications/MAMP/htdocs/d8/vendor/behat/mink/src/WebAssert.php:341
/Applications/MAMP/htdocs/d8/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php:326
/Applications/MAMP/htdocs/d8/core/modules/block_content/tests/src/Functional/BlockContentTypeTest.php:140

FAILURES!
Tests: 4, Assertions: 31, Errors: 1.

The last submitted patch, 7: 2887311-6.patch, failed testing. View results

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Status: Needs review » Needs work 
+++ b/core/modules/block_content/tests/src/Functional/BlockContentTypeTest.php
@@ -137,7 +137,7 @@ public function testBlockContentTypeEditing() {
-    $this->assertNoRaw('Body', 'Body field was not found.');
+    $this->assertFalse($this->cssSelect('tr#block-body'), 'Body field was not found.');

This is much better, much more specific in what we are testing, I would however use assertEmpty() because assertFalse would expect a boolean.

I would also update the positive assertion to match, so we know that we are not breaking coverage here (negative assertions tell us very little):

    $this->assertRaw('Body', 'Body field was found.');

to something like
$this->assertNotEmpty($this->cssSelect('tr#block-body'), 'Body field was found.');

Since we are only converting one test here I think we can be lenient in our normal 'keep changes to a minimal'-policy. This patch will still be reviewable with some additional changes.

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs work » Needs review
StatusFileSize
new 2887311-11.patch2.25 KB
new interdiff-8-11.txt1.22 KB

#10: nice catch about positive assertion! We really lost this assertion if converting as-is. Also i not found other cases in this test where the 'string' matches with html tag inside assertRaw/assertNoRaw methods.

Replace assertFalse/assertNotEmpty also makes sense. I double check the findAll(), and this method always returns array (except case where it returns result from find(), because this method can throws exceptions, but assert reacts to them correctly).

Hah, one fun moment, page has not tag with '#block-body'. So:
$this->assertFalse($this->cssSelect('tr#block-body'), 'Body field was not found.');
works like
$this->assertRaw('Body', 'Body field was found.');

Let's fix this selector. Example with '#edit-body-0-value'.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Reviewed & tested by the community

This looks pretty good to me.

+++ b/core/modules/block_content/tests/src/Functional/BlockContentTypeTest.php
@@ -108,7 +108,7 @@ public function testBlockContentTypeEditing() {
+    $this->assertNotEmpty($this->cssSelect('#edit-body-0-value'), 'Body field was found.');

Being a web test, couldn't this be $this->assertSession()->elementExists('css', '#edit-body-0-value')? I assume there was a good reason for using $this->cssSelect() (although I've never used it), so I defer to the expertise of everyone else on that point.

  • xjm committed df974e5 on 8.5.x 
    Issue #2887311 by naveenvalecha, vaplas, Lendude: Convert...

  • xjm committed f7a933d on 8.4.x 
    Issue #2887311 by naveenvalecha, vaplas, Lendude: Convert...
xjm’s picture

xjm
she/her
Primary language English
commented
Version: 8.5.x-dev » 8.4.x-dev
Status: Reviewed & tested by the community » Fixed 
+++ b/core/modules/block_content/tests/src/Functional/BlockContentTypeTest.php
@@ -1,11 +1,11 @@
-namespace Drupal\block_content\Tests;
+namespace Drupal\Tests\block_content\Functional;

At first I was like "Huh it's not changing what it extends?" but of course that's because we left the old, deprecated base classes in place, so changing the namespace is sufficient to change which BlockContentTestBase it uses.

+++ b/core/modules/block_content/tests/src/Functional/BlockContentTypeTest.php
@@ -108,7 +108,7 @@ public function testBlockContentTypeEditing() {
-    $this->assertRaw('Body', 'Body field was found.');
+    $this->assertNotEmpty($this->cssSelect('#edit-body-0-value'), 'Body field was found.');

@@ -137,7 +137,7 @@ public function testBlockContentTypeEditing() {
-    $this->assertNoRaw('Body', 'Body field was not found.');
+    $this->assertEmpty($this->cssSelect('#edit-body-0-value'), 'Body field was not found.');

Having both assertions can leave us reasonably sure that we don't need to worry about false positives/negatives, so that works for me. Thanks @Lendude for suggesting what a fix for what would have raised a red flag for me otherwise. :)

For #12, I think they're equivalent, no? So either seems fine. At least, I don't know of a particular reason to prefer one or the other, so I committed it as-is. We can file a followup if we want to change it further, I think.

Committed and pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!

Status: Fixed » Closed (fixed)

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