Posted by xjm

Problem/Motivation

This is a followup issue for #1535868: Convert all blocks into plugins.

  • The test coverage for Views' OverrideDisplaysTest is reduced in #1535868: Convert all blocks into plugins:
    +++ b/core/modules/views/lib/Drupal/views/Tests/UI/OverrideDisplaysTest.phpundefined
@@ -102,17 +99,6 @@ function testWizardMixedDefaultOverriddenDisplays() {
-    // Put the block into the first sidebar region, and make sure it will not
-    // display on the view's page display (since we will be searching for the
-    // presence/absence of the view's title in both the page and the block).

@@ -142,7 +138,6 @@ function testWizardMixedDefaultOverriddenDisplays() {
-    $this->assertNoText($view['block[title]']);

@@ -162,7 +157,6 @@ function testWizardMixedDefaultOverriddenDisplays() {
-    $this->assertNoText($new_block_title);

@@ -44,31 +44,31 @@ function testOverrideDisplays() {
-    // Change the title for the page display only, and make sure that is the
-    // only one that is changed.
+    // Change the title for the page display only, and make sure that the original title
+    // still appears on the page.

@@ -77,10 +77,7 @@ function testOverrideDisplays() {
-    $this->assertNoText($original_title);
-    $this->drupalGet('');
     $this->assertText($original_title);
-    $this->assertNoText($new_title);

Proposed resolution

  • Add back the removed test coverage.
  • Instead of assertText()/assertNoText(), use add xpath assertions.
CommentFileSizeAuthor
#10 1875344-10.patch2.43 KBaaronott
#10 1875344-interdiff.txt1.04 KBaaronott
#8 1875344-8.patch2.8 KBaaronott
#4 1875344-4.patch2.35 KBdamiankloip

Comments

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Postponed » Active
xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +Block plugins
xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +VDC
damiankloip’s picture

damiankloip commented
Status: Active » Needs review
StatusFileSize
new 1875344-4.patch2.35 KB

I think we can just add the new settings into the drupalPlaceBlock that has been added further down the test and add back the 2 missing assertions, as the initial tests are just testing it without the block.

I'm not sure we need to use xpath here? Don't block tests test region specific things?

dawehner’s picture

dawehner
Primary language German
commented
Issue tags: +Needs reroll
dawehner’s picture

dawehner
Primary language German
commented
Issue tags: -Needs tests, -Needs reroll, -VDC, -Blocks-Layouts, -Block plugins

#4: 1875344-4.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs tests, +Needs reroll, +VDC, +Blocks-Layouts, +Block plugins

The last submitted patch, 1875344-4.patch, failed testing.

aaronott’s picture

aaronott commented
Status: Needs work » Needs review
StatusFileSize
new 1875344-8.patch2.8 KB

File has been moved. This is a re-roll to accommodate that.

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/modules/views_ui/lib/Drupal/views_ui/Tests/OverrideDisplaysTest.phpundefined
@@ -132,15 +141,15 @@ function testWizardMixedDefaultOverriddenDisplays() {
-    $this->assertNoText($new_default_title);

@@ -154,7 +163,6 @@ function testWizardMixedDefaultOverriddenDisplays() {
-    $this->assertNoText($new_block_title);

Doesn't it still makes sense to ensure this assertion?

aaronott’s picture

aaronott commented
StatusFileSize
new 1875344-interdiff.txt1.04 KB
new 1875344-10.patch2.43 KB

Ugh... yes you are correct! Thanks for catching those.

dawehner’s picture

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

Thank you very much.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs tests, -Needs reroll

Committed 2514997 and pushed to 8.x. Thanks!

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Removing myself from the author field so that I can unfollow the issue. --xjm