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.

CommentFileSizeAuthor
#4 3275843-4.patch15.54 KBdanflanagan8
#2 3275843-2.patch10.27 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 3275843-2.patch10.27 KB

This module only had three mentions of classy, and I think that it was as simple as changing each one to stark. Due to local dev issues I didn't have a chance to test this patch locally. Fingers crossed!

All the other changes in the patch are changing pageText(Not)Contains to statusMessage(Not)Contains. This is a pretty trivial one.

Status: Needs review » Needs work

The last submitted patch, 2: 3275843-2.patch, failed testing. View results

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Status: Needs work » Needs review
StatusFileSize
new 3275843-4.patch15.54 KB

That's why you run tests locally...

Here's a patch that I actually tested. I don't feel like doing an interdiff. Let's just ignore the first patch.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented 
+++ b/core/modules/search/tests/src/Functional/SearchConfigSettingsFormTest.php
@@ -303,7 +304,7 @@ public function testMultipleSearchPages() {
-    $elements = $this->xpath('//*[contains(@class, :class)]//a', [':class' => 'tabs primary']);
+    $elements = $this->xpath('//div[@id="block-local-tasks"]//a');

@@ -315,7 +316,7 @@ public function testMultipleSearchPages() {
-    $elements = $this->xpath('//*[contains(@class, :class)]//a', [':class' => 'tabs primary']);
+    $elements = $this->xpath('//div[@id="block-local-tasks"]//a');

it feels like we're losing some coverage here, should we add a preprocess hook to a test module or similar to ensure that the search page order is working?

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented

Thanks for the review, @larowlan!

I'm afraid I don't understand the comment in #5:

it feels like we're losing some coverage here

Could you elaborate? Thanks! Cheers!

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave commented

Rerunning the tests for 9.5.x

But searched the search module folder for classy and nothing returned so I'll mark RTBC after the tests.

Unless you want to move back to NW for #5 afraid I don't understand either.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

  • lauriii committed f40b8e8 on 10.1.x 
    Issue #3275843 by danflanagan8, smustgrave: Search Tests should not rely...

  • lauriii committed 98df5e4 on 10.0.x 
    Issue #3275843 by danflanagan8, smustgrave: Search Tests should not rely...

  • lauriii committed 16d5d42 on 9.5.x 
    Issue #3275843 by danflanagan8, smustgrave: Search Tests should not rely...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Fixed

I don't see how the change referenced in #5 would result in lost test coverage at least to what is reasonable to test in the scope of this test. I guess it would be fine to open a follow-up if there's some test coverage that we should add back.

Committed f40b8e8 and pushed to 10.1.x and cherry-picked to 10.0.x and 9.5.x. Thanks!

Status: Fixed » Closed (fixed)

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