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
#3 3274096-3.patch6.77 KBdanflanagan8
#2 3274096-2-run-one-test.patch10.99 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 3274096-2-run-one-test.patch10.99 KB

I'm having a heck of a time trying to get this one test to pass locally. I'm posting a patch where only my trouble test runs. I've had trouble before with tests that use assertStringContainsString when there is whitespace involved.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Assigned: danflanagan8 » Unassigned
StatusFileSize
new 3274096-3.patch6.77 KB

Well forget about that anyway. I decided to stop fighting and just make a test theme that copies the necessary template from classy. Without this template there aren't wrapper divs and the markup is a mess for assertions.

This patch also contains a number of refactors to use status message assertions where appropriate. Trying to squeeze a little more value into the patch without making the review much tougher.

cliddell’s picture

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

Review

1. All references to classy have been removed:

Before

$ grep -ir classy core/modules/link/tests
core/modules/link/src/Functional/LinkFieldUITest.php:  protected $defaultTheme = 'classy';
core/modules/link/src/Functional/LinkFieldTest.php:  protected $defaultTheme = 'classy';
$

After

$ grep -ir classy core/modules/link/tests
$

2. Patch looks good:

+++ b/core/modules/link/tests/themes/link_test_theme/link_test_theme.info.yml
@@ -0,0 +1,5 @@
+name: 'Link Test Theme'

I like the idea of using a small test theme for the tests in this module. Since it eliminates dependency on any other themes which could be potentially removed in the future.

-      $this->assertSession()->pageTextContains('entity_test ' . $id . ' has been created.');
+      $this->assertSession()->statusMessageContains('entity_test ' . $id . ' has been created.', 'status');

The rest of the patch just seems to be switching out pageTextContains() assertions for statusMessageContains() like in the above example. Which IMO is much better since it targets the element which is supposed to be affected.

This is a pretty simple patch which seems to be an all around improvement and resolves the issue it was created for. RTBC!

alexpott’s picture

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

Committed and pushed e562f7ee62 to 10.0.x and 73428d6439 to 9.4.x. Thanks!

  • alexpott committed e562f7e on 10.0.x 
    Issue #3274096 by danflanagan8, cliddell: Link Tests should not rely on...

  • alexpott committed 73428d6 on 9.4.x 
    Issue #3274096 by danflanagan8, cliddell: Link Tests should not rely on...

Status: Fixed » Closed (fixed)

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