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
#10 interdiff_2-10.txt1.57 KBdanflanagan8
#10 3268443-10.patch6.79 KBdanflanagan8
#6 interdiff_2-6.txt1.33 KBravi.shankar
#6 3268443-6.patch6.67 KBravi.shankar
#2 3268443-2.patch6.67 KBdanflanagan8

Comments

danflanagan8 created an issue. See original summary.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Status: Active » Needs review
StatusFileSize
new 3268443-2.patch6.67 KB

Here's a go at it. This module was interesting in that there were a number of test themes and test modules that use classy. None of them appear to have used classy for any good reason though.

ConfigImportInstallProfileTest is an interesting case too. Stark was always the base theme, but classy is installed as part of the test because it's testing installing themes. There's nothing special about classy markup that it uses. So I just changed it to Stable.

There are at least some tests outside of the config module that use the config test modules/themes though. So let's see if everything is green elsewhere...

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Assigned: danflanagan8 » Unassigned
danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Priority: Normal » Major
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.

dww’s picture

dww
we/he/they
commented
Status: Needs review » Needs work

Before

% egrep -ir classy core/modules/config/tests
core/modules/config/tests/config_clash_test_theme/config_clash_test_theme.info.yml:base theme: classy
core/modules/config/tests/config_override_test/config/install/block.block.call_to_action.yml:    - classy
core/modules/config/tests/config_override_test/config/install/block.block.call_to_action.yml:theme: classy
core/modules/config/tests/config_override_integration_test/config/install/block.block.config_override_test.yml:    - classy
core/modules/config/tests/config_override_integration_test/config/install/block.block.config_override_test.yml:theme: classy
core/modules/config/tests/src/Functional/ConfigImportUITest.php:  protected $defaultTheme = 'classy';
core/modules/config/tests/src/Functional/CacheabilityMetadataConfigOverrideIntegrationTest.php:  protected $defaultTheme = 'classy';
core/modules/config/tests/src/Functional/ConfigEntityListTest.php:  protected $defaultTheme = 'classy';
core/modules/config/tests/src/Functional/ConfigImportInstallProfileTest.php:    $core['theme']['classy'] = 0;
core/modules/config/tests/src/Functional/ConfigImportInstallProfileTest.php:    $theme['default'] = 'classy';
core/modules/config/tests/src/Functional/ConfigImportInstallProfileTest.php:    $this->assertTrue(\Drupal::service('theme_handler')->themeExists('classy'), 'The classy theme has been installed.');

After

% egrep -ir classy core/modules/config
%

Review

  1. +++ b/core/modules/config/tests/src/Functional/ConfigEntityListTest.php
@@ -179,14 +179,14 @@ public function testListUI() {
-    $this->assertSession()->elementTextEquals('xpath', '//div[@class="layout-content"]//table/tbody/tr[@class="odd"]/td[1]', 'Default');
-    $this->assertSession()->elementTextEquals('xpath', '//div[@class="layout-content"]//table/tbody/tr[@class="odd"]/td[2]', 'dotted.default');
-    $this->assertSession()->elementExists('xpath', '//div[@class="layout-content"]//table/tbody/tr[@class="odd"]/td[3]//ul');
+    $this->assertSession()->elementTextEquals('xpath', '//div[@class="layout-content"]//table/tbody/tr[1]/td[1]', 'Default');
+    $this->assertSession()->elementTextEquals('xpath', '//div[@class="layout-content"]//table/tbody/tr[1]/td[2]', 'dotted.default');
+    $this->assertSession()->elementExists('xpath', '//div[@class="layout-content"]//table/tbody/tr[1]/td[3]//ul');

    This seems better than what we had before. Not just any 'odd' row, but the 1st row. 

  2. +++ b/core/modules/config/tests/src/Functional/ConfigImportInstallProfileTest.php
@@ -69,11 +69,10 @@ public function testInstallProfileValidation() {
+    $theme['default'] = 'stable';

    😢. stable should be going away, too. Can this be stark instead? Or at least stable9? stark would definitely be better if that can work. 

  3. +++ b/core/modules/config/tests/src/Functional/ConfigImportInstallProfileTest.php
@@ -81,7 +80,7 @@ public function testInstallProfileValidation() {
+    $this->assertTrue(\Drupal::service('theme_handler')->themeExists('stable'), 'The stable theme has been installed.');

    And here.

NW for removing 'stable' from this patch, if possible...

ravi.shankar’s picture

ravi.shankar commented
Status: Needs work » Needs review
StatusFileSize
new 3268443-6.patch6.67 KB
new interdiff_2-6.txt1.33 KB

Tried to address #5.

dww’s picture

dww
we/he/they
commented

Great, thanks! If the testbot is happy, I am. 😉

Status: Needs review » Needs work

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

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented

@ravi.shankar, thanks for jumping in here, but it's important to take a closer look at changes you are making, even if those changes have been suggested by another reviewer.

After applying the patch, there are consecutive lines in a test that read:

$this->assertFalse(\Drupal::service('theme_handler')->themeExists('stark'), 'The stark theme has been uninstalled.');
$this->assertTrue(\Drupal::service('theme_handler')->themeExists('stark'), 'The stark theme has been installed.');

Certainly one of these assertion is going to fail since stark cannot be simultaneously installed and not installed. Perhaps Schrödinger's Theme could be, but not Stark. :)

It is important to run tests locally after you have made changes.

Here are the official docs on running phpunit tests locally: https://www.drupal.org/docs/automated-testing/phpunit-in-drupal/running-...

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Status: Needs work » Needs review
StatusFileSize
new 3268443-10.patch6.79 KB
new interdiff_2-10.txt1.57 KB

Here's an updated patch that changes from stable to test_theme_theme. Essentially the only important thing is to use a theme that is not stark. test_theme_theme is a test theme with no dependencies, so it's a good candidate for something like this.

The interdiff is relative to patch #2, since #6 was a step in the wrong direction.

longwave’s picture

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

The changes all look good - as minimal as can be, the XPath changes look sensible, the changes to ConfigImportInstallProfileTest are as discussed above, and there are no remaining instances of "classy" in core/modules/config after applying this patch.

  • lauriii committed 58b15e1 on 10.0.x 
    Issue #3268443 by danflanagan8, ravi.shankar, dww, longwave:...

  • lauriii committed ddc12ad on 9.4.x 
    Issue #3268443 by danflanagan8, ravi.shankar, dww, longwave:...
lauriii’s picture

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

Committed 58b15e1 and pushed to 10.0.x. Also cherry-picked to 9.4.x. Thanks!

Status: Fixed » Closed (fixed)

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