[meta] Update tests that rely on Classy to not rely on it anymore

There's a host of wayback-in-the-day testing cleanup issues related to this problem of tests relying on markup in general and classy markup in particular. Couldn't find all of them, but here's one that included the reliance on markup (among other things) in its scope.

Given that #3082655-33: Specify the $defaultTheme property in all functional tests references classy 890 times, I suspect we're going to want/need to split this up into a meta and break up the next phase into smaller pieces. Thoughts/opinions on the most efficient and logical way to organize that would be most welcome.

Thanks!
-Derek

#3082655: Specify the $defaultTheme property in all functional tests is now in 8.8.x, so this can move forward. Yay!

Thoughts on how to organize the effort? Shall this become a meta plan w/ sub-issues per module / subsystem? It seems like it'll be hard (and unnecessary) to do this in 1 giant patch.

Thanks,
-Derek

Making the title easier to read, since I believe this blocks #3050378: [meta] Replace Classy with a starterkit theme

#6

Thoughts on how to organize the effort? Shall this become a meta plan w/ sub-issues per module / subsystem? It seems like it'll be hard (and unnecessary) to do this in 1 giant patch.

Agreed that this should be split to keep things manageable and splitting by module/subsystem seems like the most intuitive way to do so.
Some additional thoughts:

• Before creating the many sub-issues, it could be useful to review the ways that Classy is incorporated into tests other than protected $defaultTheme = 'classy'; and create a few initial sub-issues that cover the most common ways tests use Classy. Ideally, these issues would surface unknowns that would make it easier to identify blockers and correctly scope sub-issues.
• It would be beneficial to use this issue to create the issue summary template that will be used for the module-specific sub-issues. It shouldn't be a particularly challenging thing to write, but taking care of it here before using it on ~48 issues may prevent some future annoyances.

To better gauge what kind of work is required, I created three issues, all with patches awaiting review.
#3112546: Aggregator tests should not rely on Classy
#3112547: Views UI tests should not rely on Classy
#3112548: Layout Builder FuncionalJavascript tests should not rely on Classy
Ideally, these initial issues would surface concerns before we created dozens of child issues for the full effort. This is also a way to gauge the amount of work needed to accomplish this. The Views UI and Aggregator issues were relatively quick, but the Layout Builder required ~1 day of work as there was almost 40k of refactoring needed.

What does the abbreviation in the "DTT issue" tag mean? Something to do with tests, going by the tagged issues.

@andrewmacpherson I believe DTT == "Drupal Test Traits":
https://medium.com/massgovdigital/introducing-drupal-test-traits-9fe09e8...
https://gitlab.com/weitzman/drupal-test-traits

I created an issue for the user module:

There's one class that extends a class from content_translation which itself relies on classy. So this is a case where perhaps the user module would need to be postponed until the completion of content_translation. That is, if we break up the work module by module.

Before creating the many sub-issues, it could be useful to review the ways that Classy is incorporated into tests other than protected $defaultTheme = 'classy'

The thing that I noticed repeated was that tests were relying on the classes that classy places on blocks. The way I worked around that was to add an id when calling drupalPlaceBlock(). For example

$this->drupalPlaceBlock('local_tasks_block');

would be changed to this

$this->drupalPlaceBlock('local_tasks_block', ['id' => 'test_role_admin_test_local_tasks_block']);

And then later instead of keying off on the class block-local-tasks-block we would key off of the id block-test-role-admin-test-local-tasks-block

Searching the codebase for an xpath selector that relies on a class (@class) would help find these situations.

Back to this content_translation thing mentioned in #18...

The abstract class ContentTranslationUITestBase uses classy. I see 9 classes that extend ContentTranslationUITestBase, spread across 9 different modules. None of those classes are extended.

It seems like these would potentially have to be done together as a single issue.

One weird thing to point out actually, is that 2 of the 9 classes already use stark (NodeTranslationUITest and CommentTranslationUITest. Very strange.

@danflanagan8, good call on pointing out the uses of ContentTranslationUITestBase across many tests. I think it would be completely reasonable to either

• Create an issue scoped to updating the tests that extend it - update the issue summaries of any children of this issue to speicify that ContentTranslationUITestBase related tests are being addressed in another issue
• Make all the tests extending it part of the issue specific to the content translation module

The only reason for suggesting per module tickets was is seemed like a simple way to make subtasks of manageable size. If there's another logicial grouping you think would work well as an issue scope (particularly if you're able to get the work started on it), that seems fine and something I'd be happy to review.

The only reason for suggesting per module tickets was is seemed like a simple way to make subtasks of manageable size.

I think this will usually work very nicely. Especially if it comes to needing reviews from subsystem maintainers.ContentTranslationUITestBase is hopefully a unique-ish case.

I went ahead and created a ticket for this #3247901: ContentTranslationUITestBase should not rely on Classy. I'm going to try to knock that out right away.

I was working on taxonomy and found a trait that relies on classy and is used by several tests spread over several modules. I went ahead and made any issue to update the trait. It's a very simple change and will unblock several modules.

#3248309: AssertBreadcrumbTrait should not rely on Classy

For the similar issue with ContentTranslationUITestBase I opted to update all the test classes that extends that base class at the same time. I think that was the wrong approach though. It's probably easier to focus exclusively on the trait or test base in situations like this, if possible. It will make it easier to review and commit and keep moving.

I'm picking this issue back up and I finally ran into a test that really can't use stark. It's Drupal\Tests\comment\Functional\CommentCSSTest which is part of the child issue for the comment module: #3267653: Comment tests should not rely on Classy

@mglaman suggested getting standard verbiage for a @todo in such cases.

I'll start by proposing the following, but I don't feel too strongly about this.

/**
   * The theme to install as the default for testing.
   *
   * @var string
   *
   * @todo Change default theme to starterkit once it is stable. This class is
   *   testing the theme as much as it is testing this module. This class
   *   therefore cannot use stark as the default theme.
   *   https://www.drupal.org/project/drupal/issues/3050378
   */
  protected $defaultTheme = 'classy';

Or we could go really simple.

/**
   * The theme to install as the default for testing.
   *
   * @var string
   *
   * @todo Change default theme to starterkit once it is stable.
   *   https://www.drupal.org/project/drupal/issues/3050378
   */
  protected $defaultTheme = 'classy';

Thoughts?

I'd go with:

/**
   * The theme to install as the default for testing.
   *
   * @var string
   *
   * @todo This test's reliance on [whichever of markup, classes, data attributes, etc.] makes Stark
   *   a bad fit as a base theme. Change the default theme to Starterkit once it is stable.
   *
   *  @see https://www.drupal.org/project/drupal/issues/NEW_FOLLOWUP_NID_HERE
   */
  protected $defaultTheme = 'classy';

That, but wrapped properly etc. Rather than linking the starterkit meta, we should file explicit followups for the individual tests as children of this meta, and postpone them on the Starterkit meta.

+1 I like xjm suggestions. Especially opening the follow up right away as postponed so we can commit the link to it.

Right now these are being scoped by module, which is not usually a preferred scoping pattern. To some extent it makes sense because different modules have different expectations about markup, but there are also a number of tests in most of the patches so far where the base theme can be changed to Stark with no other changes needed.

Could we get a single patch that changes every "Classy" to "Stark", eliminate the change from tests that fail as a result, and commit the remainder that need no change in a single patch?

Could we get a single patch that changes every "Classy" to "Stark", eliminate the change from tests that fail as a result, and commit the remainder that need no change in a single patch?

This could be problematic if there is a negative assertion relying on Classy that does not have a corresponding positive assertion relying on Classy. Like maybe there's an assertion for no error message being present that relies on Classy selectors. Such a test would still pass with Stark, but we would be losing the coverage.

It's painful, but I think that it's important to search for css and xpath in every test being changed from Classy to Stark and checking that none of the selectors for negative assertions look fragile.

@danflanagan8, good point; that's a good reason to only change a few tests at a time.

I've glanced at a number of the remaining modules and something that is going to come up time and again as updating assertions related to error messages and status messages.

Things like...

$this->assertSession()->elementNotExists('xpath', '//div[contains(@class, "messages--error")]');

$xpath_err = '//div[contains(@class, "error")]';
    $this->assertNotEmpty($this->xpath($xpath_err), 'Enabling translation only for entity bundles generates a form error.');

$this->assertSession()->elementNotExists('xpath', '//div[contains(@class, "messages--status")]');

$this->assertSession()
          ->elementContains('css', '.messages--warning', node_get_type_label($node) . ' content items were skipped as they are under moderation and may not be directly published.');

And a couple examples from Functional JS tests where we wait for messages.

$assert_session->assertNoElementAfterWait('css', '.messages--warning');

// Check that the 'content has been updated' message status appears to confirm we left the editor.
    $assert_session->waitForElementVisible('css', 'messages messages--status');

I would like to propose building out an AssertMessageTrait (maybe two if the one that waits needs to be its own thing) that is designed to handle stuff like this. It would assert existence and non-existence or errors or statuses or warnings. It could assert the message text if needed. And it could wait for a kind of message or a message with a specified text.

This is in part inspired by the AssertBreadcrumbTrait whose existence has made this classy->stark switch much easier in several cases.

I went ahead and made an issue for my idea in #30 and have a patch posted: #3268932: Add methods to assert status messages to WebAssert

I'm going to start advertising the NW child issues. I've added some advice for reviewing child issues to the IS. It's something at least.

Regarding inline_form_errors, there's currently only one mention of classy, and it's in a test that is about to be moved into the ckeditor module (which itself is moving out of core): #3271046: Move integration test for CKEditor 4 and Inline Form Errors into CKEditor 4

So we don't need a child issue about inline_form_errors, which is cool.

There are a handful of other modules we can ignore since they are also moving out of core. I'll list them per #3118154: [meta] Deprecate dependencies, libraries, modules, and themes that will be removed from Drupal 10 core by 9.4.0-beta1

• aggregator
• ckeditor
• quickedit
• color
• tracker
• forum
• rdf
• hal

There are a few others that are maybe being removed, but the list above looks like the ones we can definitely ignore from a classy-to-stark standpoint.

p.s. As one of the suckers who volunteered to deal with one of those deprecated modules, I'd very much welcome help while folks are still thinking of this problem space to untangle quickedit tests from classy, too. 😉 I'd guess others would agree. So yes, those all could be ignored, but it'd be great if they weren't. 😅

@dww, good point! I wonder if it's easier to do that while the module is still in core? Or if it's best accomplished as an issue in the contrib version of the module?

Moving classy out of core is likely going to affect a TON of contrib modules with tests that use classy. But the quick fix boils down to:

1. Adding classy (contrib) to compser.json
2. Adding @requires classy (or something close to that) to the annotation of any test relying on classy.

Added a link to the IS to a google sheet I just made. Time to get serious!

https://docs.google.com/spreadsheets/d/1OAAnJ7Q3CcM9oeGyXkMrgw-6QjAYEkkn...

Added #3276652: AssertMenuActiveTrailTrait should not rely on classy/bartik (which has been affecting making Olivero the default theme).

Created all the missing issues for the remaining modules. Currently still creating the system module sub-issues.

What are we going to do with tests such as \Drupal\Tests\system\Kernel\Render\ClassyTest we'll just remove them?

Since we are few weeks from the Drupal 10.0.0-beta1 deadline and Starterkit Theme is on the verge of being marked stable, I think we should do a intermediate step of switching to use starterkit theme on all of the remaining tests: #3304731: Update remaining tests using Classy to use Starterkit.

#39: Let's convert those Classy tests to directly test Starterkit theme instead. They were testing a specific regression in Classy so I guess they are relevant for Starterkit which is based on Classy.

I think we can mark this done? 😊

I think we still want to do the work to use stark instead of starterkit where possible. That work should probably happen under a new meta though.

Should the child issues that are still open be updated to use starterkit

Could this be closed? When searching for

$defaultTheme = 'classy';

in 10.1.x I get 0 hits?

believe this has been completed.

There were three outstanding postponed children listed in the issue summary. However, I checked and confirmed that all three were actually already fixed in either #3304731: Update remaining tests using Classy to use Starterkit or #3082655: Specify the $defaultTheme property in all functional tests.

Saving (belated) credits for the issue management on this. Thanks!

This was fixed for 10.0.0