Problem/Motivation

Because of #3083275: [meta] Update tests that rely on Classy to not rely on it anymore and Classy being deprecated in Drupal 9 + removed in Drupal 10,: Tests that aren't specifically testing Classy yet declare $defaultTheme = 'classy'; should be refactored to use Stark as the default theme instead.

Proposed resolution

Change all tests in this module to use Stark as the default theme, and refactor the tests where needed so they continue to function properly.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#11 interdiff_9-11.txt496 bytesdanflanagan8
#11 3271507-11.patch43.43 KBdanflanagan8
#9 interdiff_4-9.txt5.41 KBdanflanagan8
#9 3271507-9.patch43.23 KBdanflanagan8
#4 interdiff_2-4.txt737 bytesdanflanagan8
#4 3271507-4.patch43.26 KBdanflanagan8
#2 3271507-2.patch43.26 KBdanflanagan8

Comments

danflanagan8 created an issue. See original summary.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Assigned: danflanagan8 » Unassigned
Status: Active » Needs review
StatusFileSize
new 3271507-2.patch43.26 KB

This is a fairly big one. But to the extent that any of these issues are exciting, this one is exciting to me. That's because it's the first one where I get to use the brand new status message assertions! #3268932: Add methods to assert status messages to WebAssert

So that's one thing you'll see here. I refactored to use those status message assertions where possible. I think only one of them NEEDED to be refactored, but the new assertions are more readable while being slightly more robust so it's an easy win.

Other changes are fairly typical of these Classy-to-Stark issues, like selecting block elements using the id instead of a class.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented

Something that is new to this Classy-to-Stark issue is that there were lots of selectors for block regions. In one instance I made a little map of the corresponding selectors. This is basically the mapping I used anywhere I had to make changes to selectors for regions.

+++ b/core/modules/block/tests/src/Functional/BlockTest.php
@@ -373,15 +373,22 @@ public function moveBlockToRegion(array $block, $region) {
+    $region_xpath = [
+      'header' => '//header[@role = "banner"]',
+      'sidebar_first' => '//aside[contains(@class, "layout-sidebar-first")]',
+      'content' => '//div[contains(@class, "layout-content")]',
+      'sidebar_second' => '//aside[contains(@class, "layout-sidebar-second")]',
+      'footer' => '//footer[@role = "contentinfo"]',
+    ];
+

Here's another bit of code I wanted to call out.

+++ b/core/modules/block/tests/src/Functional/BlockUiTest.php
@@ -92,8 +92,8 @@ protected function setUp(): void {
-    $this->clickLink('Demonstrate block regions (Classy)');
-    $this->assertSession()->elementExists('xpath', '//div[contains(@class, "region-highlighted")]/div[contains(@class, "block-region") and contains(text(), "Highlighted")]');
+    $this->clickLink('Demonstrate block regions (Stark)');
+    $this->assertSession()->elementExists('xpath', '//header[@role = "banner"]/div/div[contains(@class, "block-region") and contains(text(), "Header")]');

Obviously I'm changing Highlighted to Header here. But the "Highlighted" region does not appear to be special in any way. So I changed to a region that is easier to select with stark.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
StatusFileSize
new 3271507-4.patch43.26 KB
new interdiff_2-4.txt737 bytes

try that again

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Issue tags: +Drupal 10

Updating priority to Major just like @xjm did for #3248295: Taxonomy tests should not rely on Classy and adding D10 tag.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Needs work 
  1. +++ b/core/modules/block/tests/src/Functional/BlockSystemBrandingTest.php
@@ -38,19 +38,19 @@ protected function setUp(): void {
+    $site_logo_xpath = '//div[@id="block-site-branding"]/a/img';

    we could look for the image src too? That would be similar to the next selector that looks for a specific text content. 

  2. +++ b/core/modules/block/tests/src/Functional/BlockSystemBrandingTest.php
@@ -38,19 +38,19 @@ protected function setUp(): void {
+    // The slogan cannot be selected so we simply check for the text.
+    $this->assertSession()->elementTextContains('xpath', '//div[@id="block-site-branding"]', 'Community plumbing');

    Haven't tried it in the test itself but a xpath selector to select a text node could be 

    //*[@id="block-stark-branding"]/descendant::text()[last()]

  3. +++ b/core/modules/block/tests/src/Functional/BlockUiTest.php
@@ -354,16 +356,13 @@ public function testBlockPlacementIndicator() {
-    $arguments = [':message' => 'Only digits are allowed'];
-    $pattern = '//div[contains(@class,"messages messages--error")]/div[contains(text()[2],:message)]';
-    $elements = $this->xpath($pattern, $arguments);
-    $this->assertNotEmpty($elements, 'Plugin error message found in parent form.');
+    $this->assertSession()->statusMessageContains('Only digits are allowed', 'error');

    nice one :)

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs work » Needs review

actually probably doesn't need a NW, just some small comments

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented

Thanks for the review @nod_!

Here's a patch with 6.1 and 6.2 addressed. Thanks for the xpath selector in 6.2. That's pretty slick.

And thanks for 6.3. That's actually a case I included in the CR for the brand new methods for asserting status messages: https://www.drupal.org/node/3270424

I also noticed that I had removed a reference to classy in BlockDemoTest::testBlockDemo that I should not have removed. I have added it back. That is literally test coverage for classy and it needs to remain as long as classy is in core.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
StatusFileSize
new 3271507-9.patch43.23 KB
new interdiff_4-9.txt5.41 KB

Of course I forgot the patch...

Status: Needs review » Needs work

The last submitted patch, 9: 3271507-9.patch, failed testing. View results

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Status: Needs work » Needs review
StatusFileSize
new 3271507-11.patch43.43 KB
new interdiff_9-11.txt496 bytes

Oops. I guess I outsmarted myself. We do indeed need to remove classy from the list of themes in BlockDemoTest::testBlockDemo. Classy does not have a block demo page because it is marked as "hidden", thus the 404. Previously it was getting removed from the array of themes automatically since it was the default theme. So I'm re-removing classy.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Reviewed & tested by the community

All good for me, thanks

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed f88b4a9a8a to 10.0.x and 1876f3f5b1 to 9.4.x. Thanks!

  • alexpott committed f88b4a9 on 10.0.x 
    Issue #3271507 by danflanagan8, nod_: Block tests should not rely on...

  • alexpott committed 1876f3f on 9.4.x 
    Issue #3271507 by danflanagan8, nod_: Block tests should not rely on...

Status: Fixed » Closed (fixed)

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