Specify the $defaultTheme property in all functional tests

Wim Leers created an issue. See original summary.

#2352949: Deprecate using Classy as the default theme for the 'testing' profile is green now. Let's get this going. First, let's get a list of all the failing tests if we disallow this deprecation in Drupal core.

It looks like most of the tests can actually switch to stark 🥳

So far, only one tests needed to continue to use classy.

I think this issue and patch are simple enough that a novice contributor can continue this 😊

Here's one more, so I have an excuse to roll this on top of #2352949-82: Deprecate using Classy as the default theme for the 'testing' profile :)

The next step after this: #3083275: [meta] Update tests that rely on Classy to not rely on it anymore.

Rebased on top of #2352949-92: Deprecate using Classy as the default theme for the 'testing' profile, which is RTBC 🥳

#2352949: Deprecate using Classy as the default theme for the 'testing' profile landed! 🥳

Rebased.

This is now ready for somebody new to Drupal core contribution to take completion!

Hi xjm,

I just analysed the test failers of latest patch. It shows ~1500 test failers . So can we create child issues per module? Thanks

Re: #13: I don't think we want to split this up. It's super trivial boiler-plate that needs to happen.

However, the key piece is this:

diff --git a/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php b/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
index f9e346ac4c..3c76756bd5 100644
--- a/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
+++ b/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
@@ -140,8 +140,6 @@ public static function getSkippedDeprecations() {
       // This deprecation comes from behat/mink-browserkit-driver when updating
       // symfony/browser-kit to 4.3+.
       'The "Symfony\Component\BrowserKit\Response::getStatus()" method is deprecated since Symfony 4.3, use getStatusCode() instead.',
-      // @todo Remove in https://www.drupal.org/project/drupal/issues/3082655
-      'Drupal\Tests\BrowserTestBase::$defaultTheme is required in drupal:9.0.0 when using an install profile that does not set a default theme. See https://www.drupal.org/node/2352949, which includes recommendations on which theme to use.',
     ];
   }

That's what turns on the deprecation errors that shows you what's still missing. We'd have to wait to do that until all the child issues are resolved, but without including that part, we wouldn't know if each child issue was complete.

Plus, it's just a ton of issue churn, separate reviews, test bot runs, etc. I believe it'll ultimately be faster and easier to simply do it in 1 big patch. There shouldn't be any discussion needed. If you're changing assertions in this issue, You're Doing It Wrong(tm). ;) Any of that belongs in #3083275: [meta] Update tests that rely on Classy to not rely on it anymore. The only things in here should be setting tests to either explicitly use stark (if possible) or classy (if necessary).

That said, if someone wants to spin up a sandbox to work on this, I'm not opposed. But I don't think sandboxes get automated testing, so that probably wouldn't help much.

If someone wants to add a list of modules to the Remaining tasks portion of the summary so we can cross them off as we complete modules, that'd be fine, too.

Personally, I'd try writing a script to set *everything* to use stark and see how many failures we still have. Anything that fails as stark should be set to classy and we should be RTBC.

At least, that's my take on the simplest path forward here.

Cheers,
-Derek

Yes, one issue please. There are no semantical or conceptual differences between the tests in various modules. It's the same pattern applied across all tests: try to use stark if you can (if it doesn't require changes in assertions), otherwise use classy. As is explained briefly in #3 and in detail in the change record.

There is a person who reached out to me in Drupal Slack (username: DrupalTree). They asked a lot of questions, I answered all of them. They haven't posted here yet though.

#14 is right. But personally I'd do the copy/paste work faster than I can write that script. :)

Hi I am working on this issue.

Thanks, @iyyappan.govind! (Turns out he is the DrupalTree person I mentioned in #15!)

In #2352949: Deprecate using Classy as the default theme for the 'testing' profile we changed our minds on what this should be called...

@iyyappan.govind - yes, thanks for taking this on!

@Wim Leers - whatever's faster. ;)

Cheers,
-Derek

Hi I have added the $defaultTheme property to below 14 modules.

action
aggregator
ban
basic_auth
big_pipe
block
block_content
block_place
book
ckeditor
color
comment
config
config_environment

Thanks

Good job! That brought us from 1526 failures in #7 to 1449 failures in #19. Progress! 🎉 👏 Thank you :)

Hi @wimleers, Thanks for the appreciation. I just re-rolling the block and block_content module function $defaultTheme from stark to classy.

Re-rolled comment, config modules to classy and added stark as $defaultTheme in jsonapi module. Thanks

@iyyappan.govind thanks for continuing to work on this! 2 things:

1) It's not exactly clear from your comments if you're doing this, but please don't change entire modules as stark vs. classy. The intention of this issue is to try to get as many tests as possible using stark. If individual tests need to be rolled back to classy to pass, so be it, but we want to leave as many tests using stark as we can.

2) Please attach interdiff files so we can more easily try to see what's happening and make sure you're on the right track. For example, I'm attaching the ones between #19 and #21, and between #21 and #22.

Thanks!
-Derek

Added the $defaultTheme to jsonapi module. Thanks

Hi I have added the $defaultTheme = Stark in all the core module test files. Just adding the patch to review. Thanks

Fixed the syntax error. Updated the patch.

Hi Re-rolled the patch. Thanks

Removed the binary file entry on the patch. Thanks

EPIC progress here, @iyyappan.govind! 👏👏👏👏

@iyyappan.govind asked me to help him with the last 10 failures.

1. The BuildTestTest::testPortMany() failure is most certainly unrelated. Also, I cannot reproduce it locally.
2. The failures in GetTestMethodCallerExtendsTest and GetTestMethodCallerTest were legitimate, we need to update their assertions slightly.
3. The failure in PageCacheTagsIntegrationTest was caused by the addition of $defaultTheme: this is one of the few tests in core where that's not necessary, because it uses the standard install profile, which contains configuration that already sets a specific default them. Easy fix: revert the change :)
4. Same in BreadcrumbTest.
5. WorkspaceSwitcherTest failed because it was using stark but had to use classy. Fixing this also required removing WorkspaceTestUtilities::$defaultThem, which did not really make sense to add anyway (the default theme should be set on individual test classes or base test classes, not on traits).
6. DrupalSelenium2DriverTest just didn't have $defaultTheme yet :)

Also fixed one coding standard violation in EditModeTest (one extraneous blank line).

Apparently this needs to be created with --binary because of git diff being braindead:

error: cannot apply binary patch to 'core/modules/jsonapi/tests/src/Functional/MenuLinkContentTest.php' without full index line

Rerolled #30.

I apparently omitted the change I mentioned in #30.3. Not sure how that happened. But yeah, I've been doing this for so long and I still make silly mistakes as you can see 😇

Hi @wimleers,

Thank you for your help on this.

Fantastic work, here! THANK YOU @iyyappan.govind and @Wim Leers!!!

I asked at #2659890-36: [Policy] [Plan] Drupal 9 and 10 markup and CSS backwards compatibility about how @xjm (release manager) and @lauriii (front end manager) want to handle this issue. There was talk of adding @todo @see comments at every reference to classy that would point to #3083275: [meta] Update tests that rely on Classy to not rely on it anymore. I'm hoping they can clarify if that's really what they want, and if so, exactly what they want the comments to look like, so we can get this RTBC and committed.

Onward! ;)
-Derek

#35: I don't think this needs such a @todo comment for every single occurrence of $defaultTheme = 'classy';. Some of them will turn out to be legitimate/necessary. Besides, we won't forget to do this anyway, since the follow-up already exists.

That's before I read your comments at #2659890-36: [Policy] [Plan] Drupal 9 and 10 markup and CSS backwards compatibility and #3083275-4: [meta] Update tests that rely on Classy to not rely on it anymore: this patch sets Classy as the default theme 890 times. Stark is fortunately set as the default theme an order of magnitude more: 7145 times. Still, that's indeed still a lot of Classy.

I too defer to release managers and the front end framework manager then 🙂

I'll do some spot testing on those 890 Classy uses: I'll try to bring that number down. Nonetheless, I'm already quite happy with this significant progress in the right direction :)

Apparently @iyyappan.govind spent a good portion of his 3 days of holidays last week on this issue! 🤯😳

We should all be very thankful to @iyyappan.govind! 🙏

Hi Wimleers, Really thanks for appreciating me. Your appreciation gives me a motivation to contribute more on the Drupal. Thanks again.

I'll do some spot testing on those 890 Classy uses: I'll try to bring that number down. Nonetheless, I'm already quite happy with this significant progress in the right direction :)

Did that. I tried switching the following from protected $defaultTheme = 'classy'; to protected $defaultTheme = 'stark';:

1. \Drupal\Tests\book\Functional\BookContentModerationTest
2. \Drupal\Tests\block_content\Functional\BlockContentListTest
3. \Drupal\Tests\dblog\Functional\DbLogViewsTest
4. \Drupal\Tests\image\Functional\ImageDimensionsTest
5. \Drupal\Tests\big_pipe\Functional\BigPipeTest
6. \Drupal\Tests\node\Functional\NodeTypeTest
7. \Drupal\Tests\contact\Functional\ContactStorageTest
8. \Drupal\Tests\system\Functional\Entity\EntityViewControllerTest
9. \Drupal\Tests\media\FunctionalJavascript\MediaSourceImageTest
10. \Drupal\Tests\page_cache\Functional\PageCacheTest

All of them except that last one resulted in a failing test. Meaning 9 out of the 10 I tested were correctly using classy 👍 Many of them could easily be switched from Classy to Stark, but that's what we have #3083275: [meta] Update tests that rely on Classy to not rely on it anymore for. This patch is really hard to keep applying to HEAD: any test file rename or test addition forces this patch to be updated! So I think this is ready 🚢

#31:

Apparently this needs to be created with --binary because of git diff being braindead: […]

This happened again 😬😬😬😬

Also, recent HEAD changes broke this patch again.

Something went wrong in #40's rebase evidently.

1. 🦅👁 Fixed! ✅
2. GREAT find! 👏Discussed with Lauri. This seemed like a pointless follow-up: fixing those here would be less work and barely increases scope. Lauri was fine with that. Lauri asked to also grep the code base for more cases where a default theme is being configured. All of the other cases are either kernel tests (unaffected by this) or they're setting a default theme for individual test methods, rather than the entire test class. Except for one case, which I also updated. ✅
3. 🙂

_{Also rebased again…}

🙃 4 fails, 4 new tests since I rolled a fresh patch against head 🙃

Worst chasing of head I’ve ever had I think — fortunately it’s not complex!

In the past ~14 hours a number of new tests were added… but none of them tests that need this! So, the re-test of #47 should still pass all tests 🤞🤓

NICE I just trolled myself there. The sole failure was added by #3084069: Allow marking themes as experimental, which I helped get to RTBC 😅😂 Will rebase once more.

Another test run, another fail triggered by a newly added test 🧐 This time #3081587: Multilingual content is shown double in the media library view.

In the past 15 minutes two more core commits happened, so another rebase. 😬

Discussed with core committers.

I realized that committing this would break every single RTBC patch that adds a new functional test. That’s far too disruptive just before alpha. So I now think the smarter strategy is to core committers agree that this patch is okay to land after alpha-deadline issues.

@larowlan, @alexpott, @catch committers were positive about this, and @xjm suggested tagging this beta target. So, did that :)

This should be scheduled for some date after beta 1, so the week of the Nov. 11.

Re-rolled the patch - I expect any new tests added in the interim to fail.

I was going to work on making this green again, but per #58 that should only happen next week.

Okay we need different patches on 8.x.x and 9.0.x due to differences in \Drupal\Tests\Listeners\DeprecationListenerTrait::getSkippedDeprecations - the patch in #59 is good for 9.0.x but here are new clearly labelled patches.

Fixed the new tests... The interdiff is the same for both the 9.0.x and 8.x.x patches.

Hmmm... for some reason Drupal\Tests\media_library\FunctionalJavascript\TranslationsTest is going to fail. I thought it was due to #3087036: third_party_settings in THEME.settings.yml cannot be dependency managed but it's due to something else... wow!

Ah I see the tests depend on classy's markup...

Weird that the testbot didn't mark this needs work automatically...

Trivial changes to fix tests that rely on classy and add $defaultTheme to another new test.

Woo hooo! Green patches again. Let's land this sucker. ;)

Thanks!
-Derek

p.s. I'm now preemptively defining defaultTheme in new test classes I'm adding, so they/this don't have to be re-rolled again. See #3092125-14: Add test coverage of Views RSS feeds with translated content for example...

I agree with the RTBC of #69. See my original RTBC in #39, which includes a detailed rationale. @alexpott in his recent rerolls has followed the same rationale: start with stark, but change to classy when that causes test failures.

#70: 👍

Great!! Excited to see the green patches again. : 👍

Needs one more re-roll (I'm hoping to commit this today just before the beta is tagged).

error: core/modules/system/tests/src/Functional/Path/UrlAlterFunctionalTest.php: does not exist in index
error: core/tests/Drupal/FunctionalTests/Hal/PathAliasHalJsonAnonTest.php: does not exist in index
error: core/tests/Drupal/FunctionalTests/Hal/PathAliasHalJsonBasicAuthTest.php: does not exist in index
error: core/tests/Drupal/FunctionalTests/Hal/PathAliasHalJsonCookieTest.php: does not exist in index
error: core/tests/Drupal/FunctionalTests/Rest/PathAliasJsonAnonTest.php: does not exist in index
error: core/tests/Drupal/FunctionalTests/Rest/PathAliasJsonBasicAuthTest.php: does not exist in index
error: core/tests/Drupal/FunctionalTests/Rest/PathAliasJsonCookieTest.php: does not exist in index
error: core/tests/Drupal/FunctionalTests/Rest/PathAliasXmlAnonTest.php: does not exist in index
error: core/tests/Drupal/FunctionalTests/Rest/PathAliasXmlBasicAuthTest.php: does not exist in index
error: core/tests/Drupal/FunctionalTests/Rest/PathAliasXmlCookieTest.php: does not exist in index

Rerolled - not sure if there are any new tests but the testbot can tesll us that. git did a 3-way merge.

Several linear metres of PHPCS terminal output later...

Committed/pushed to 9.0.x, 8.9.x and 8.8.x, thanks!

Several linear metres of PHPCS terminal output later...

ROFL 🤣

Not very surprising though because IIRC this took git more than a minute to apply. On a very recent machine. 🙃