Update System module tests to not use Bartik and Seven

Assign this issue to the parent #3285205: [META] Convert test that use Bartik/Seven to Olivero/Claro.

The migration tests need to continue to use bartik and/or seven. The later test will not ever need a change, it is the first one that will require some work when bartik and seven exist in contrib. In terms of this issue there should be no change.

• 4 core/modules/system/tests/src/Kernel/Migrate/d7/MigrateThemeSettingsTest.php
• 2 core/modules/system/tests/src/Kernel/Plugin/migrate/source/d7/ThemeSettingsTest.php

I have updated the IS.

Formatting

We need to do the migrate tests too in order to be able to remove the themes from core in 10.x. Might need to be split out into their own issue.

I missed this issue for covering the tests in the system.module. I have made the changes for the theme setting migration over in #3281427: Update Block and Theme setting migrations to not use Bartik and Seven in order to keep the migration work together.

The IS mentions a source plugin test, core/modules/system/tests/src/Kernel/Plugin/migrate/source/d7/ThemeSettingsTest.php. While we could change the test it is not necessary because the strings being used are examples of what is in the D7 database. We could save ourselves a wee bit of work.

Here are two of the tests updated.
core/modules/system/tests/src/Functional/System/ThemeTest.php
core/modules/system/tests/src/Functional/Theme/ThemeTokenTest.php

updated the patch and tried resolving all the issues mention above

Thanks for the update, few things:

1. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/BrokenCacheUpdateTest.php
   @@ -17,7 +17,7 @@ class BrokenCacheUpdateTest extends BrowserTestBase {
   +  protected $defaultTheme = 'classy';
   

   why change this? stark should be fine as-is

2. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdatePathTestBaseFilledTest.php
   @@ -18,7 +18,7 @@ class UpdatePathTestBaseFilledTest extends UpdatePathTestBaseTest {
   +  protected $defaultTheme = 'classy';
   

   same here, why the change?

3. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdatePathTestBaseFilledTest.php
   @@ -404,9 +404,8 @@ public function testUpdatedSite() {
        $expected_enabled_themes = [
   -      'bartik',
   -      'seven',
   -      'stark',
   +      'claro',
   +      'classy',
   

   Here I would expect to have olivero, claro, stark in that order. Not only claro and classy.

Hey @nod_ I will work on the given changes

Updated with changes please have a look.

I recently installed a new 10.x.dev version of drupal and at a fresh install when i ran this script to check the bartik and seven is present i found only this two files

git grep -E '(bartik)|(seven)' -- 'core/modules/system/tests' | awk -F: '{print $1}' | sort | uniq -c
      1 core/modules/system/tests/src/Functional/Form/ValidationTest.php
      3 core/modules/system/tests/themes/test_subseven/test_subseven.info.yml

So in the Form/ValidationTest.php it is the 'seven' as Textfield as '7seven' which is not particulary based on core seven theme.
And regarding the second one that is a test-sub module . I Think we should have a disscussion around to remove this submodule or just keep it.
Well other than that everything is fine with the system module , No further changes are needed.
Moving this issue to 'Needs Review'.

I did a mistake in here, I didn't know that the patch was already applied so still previous issue exists.

The patch which was failed at #18, So for it I collaborated with @aarti-zikre and we came to a conclusion that their are some default values which are been set in the DumpDatabase. So we think that we need to change to Database Dump files because it has bartik and seven set as default on which this tests are running and probably failing as well.

The tests are using the default database dump files of 9.3.0 , Which we think should have a newer drupal 10 version.

So does anyone have any idea how can we change the DatabaseDumpFiles ?

Any Help Welcome !!

The db dump mentioned in #21 were update last as part of https://www.drupal.org/project/drupal/issues/3275093. Tagging @catch @larowlan for suggestions.

Discussion happen with @catch and @larowlan around this in drupal slack https://drupal.slack.com/archives/C014CT1CN1M/p1659678532583749 where we came to following conclusion:

So to summarise:

• Load existing dump
• Switch themes.
• Export the dump with latest theme value.
• Replace the existing dump file with new one

Note (from catch) One more thing the 9.3.0 dump isn't in 9.3.0, so you need to get that from 10.1.x, then load it into the 9.3.0 site.

I'm unsure what to do for the 9.5.x version:

I _think_ we shouldn't include the updated fixture, since we're not removing bartik/seven in 9.x.
That would however mean that we need to mark (a part of) UpdatePathTestBaseFilledTest as @group legacy.

I _do_ think we need to update the fixture in #3290810: Remove updates added prior to 9.4.0 (9.4.4 for ckeditor) and add 9.4.0 database dumps to have claro/olivero as default themes. That 9.4.0 fixture will not be tested against in Core tests.

I'll leave that for Bigger Brains to decide.

1. +++ b/core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTrayTestBase.php
   @@ -156,10 +156,10 @@ protected function assertOffCanvasBlockFormIsValid() {
   +    // Remove 'claro' theme. Settings Tray "Edit Mode" will not work with this
   +    // theme because it removes all contextual links.
        return array_filter(parent::getTestThemes(), function ($theme) {
   -      return $theme !== 'seven';
   +      return ($theme !== 'claro');
        });
   

   I can see contextual links with claro, but the test fails when we don't remove it. Probably a follow-up to check out what's going on

2. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdatePathTestBaseFilledTest.php
   @@ -84,9 +84,6 @@ public function testUpdatedSite() {
   -    // Make sure the translated slogan appears.
   -    $this->assertSession()->pageTextContains('drupal Spanish');
   -
   

   not sure what's the reason for removing this test.

I do not have the answer to #27, in any case if I can get an answer about the drupal Spanish thing, I could RTBC. It's progress.

That would however mean that we need to mark (a part of) UpdatePathTestBaseFilledTest as @group legacy.

I thought all upgrade path tests were @group legacy anyway? If not, then I'm neutral on either having the 10.x and 9.5x test coverage match (via Claro and Olivero), vs. just marking the 9.5.x test legacy and keeping the Bartik/Seven coverage until it's out of support, we could always go the @group legacy route here and change our minds later (in a different issue) if there's a problem.

Agreed the 9.4.0 update should have claro/olivero (and just be backported to 9.5.x for contrib to use, not for core tests).

if I can get an answer about the drupal Spanish thing, I could RTBC. It's progress.

@nod: AFAICT olivero doesn't provide slogan out of the box (at least I couldn't find where to enable it).
That means with the current setup of olivero, there's no slogan, so there's no translated slogan (which is what the removed assertion was looking for).

Hope that clarifies stuff?

I thought all upgrade path tests were @group legacy anyway?

@catch:

/**
 * Runs UpdatePathTestBaseTest with a dump filled with content.
 *
 * @group #slow
 * @group Update
 */
class UpdatePathTestBaseFilledTest extends UpdatePathTestBaseTest 

and

  /**
   * Tests that the content and configuration were properly updated.
   */
  public function testUpdatedSite() 

I can see contextual links with claro, but the test fails when we don't remove it. Probably a follow-up to check out what's going on

Follow up create here: #3302652: Claro fails contextual links related tests

Looks like rtbc to me

Committed/pushed to 10.1.x and cherry-picked to 10.0.x

Let's try a 9.5.x version with @group legacy on the database update tests instead of changing the database dump, and see how that goes.

It is disappointing to see changes to the source plugin test, ThemeSettingsTest, when I pointed out in the Issue Summary that that was being done elsewhere and in #9 that they are not needed. The values changed are to represent data in the D7 source database. And as far as I know olivero does not have a D7 version and thus the strings are now wrong. It does not invalidate the test but it will cause a future maintainer to scratch their head for a bit. This is an example of why I have preferred to have the migration tests done in separate issue.

I have attempted to update the patch per #35. I added @legacy to core/modules/system/tests/src/Functional/UpdateSystem/BrokenCacheUpdateTest.php and core/modules/system/tests/src/Functional/UpdateSystem/UpdatePathTestBaseFilledTest.php. No interdiff because of the tar.gz in the earlier patch, #26.

sorry I think I did not understand correctly. Maybe a revert is a better solution for now?

@nod_ basically it's a kernel test with sample data, so it wouldn't break if we remove bartik/seven from core and could have kept the same theme name (since that's closer to what the actual data is). However, I discussed whether to revert that hunk with @quietone and she said it doesn't particularly matter - it would only be potentially confusing to someone working out what the example data in the test represents, and few people are going to be working on Drupal 6/7 theme settings migrations these days (or at all, since it's not a high priority in any migration compared to content entities). Given that, we can just continue with the 9.5.x backport here and leave the test change as-is.

@catch:

I _think_ @nod is referring tot the remark by @quietone:

It is disappointing to see changes to the source plugin test, ThemeSettingsTest, when I pointed out in the Issue Summary that that was being done elsewhere and in #9 that they are not needed. The values changed are to represent data in the D7 source database. And as far as I know olivero does not have a D7 version and thus the strings are now wrong. It does not invalidate the test but it will cause a future maintainer to scratch their head for a bit

I do feel (partly) responsible for that, got in halfway this issue and fixed the test-failures, didn't (really) read all the comments.

To make this right I see 2 options:

1) Revert this commit, make new patches without the unneeded/unwantend changes in ThemeSettingsTest.
2) Create a follow-up with patches for 10.0.x and 10.1.x reverting the unneeded/unwantend changes in ThemeSettingsTest and not have these changes in the 9.5.x patch in this issue.

I _think_ the cleanest option would be 1), but am open for any other options.

@nod_, @Spokje, thanks. I see that I should have more explicit in that there is no need to revert the changes to that source plugin test, per the reasons in catch's comment.

Right, let me try to further complicate the issue :)

I _think_ we don't need to put @group legacy on the database update tests in _this_ issue.

Per the documentation and previous deprecation/removal issues we deprecate in the deprecation issue.
_If_ for whatever reason we end up not deprecating bartik in 9.x, we need to un-deprecate them.
Also, since bartik isn't currently deprecated, there's no (technical) reason to deprecate straight away.

So I propose to create a 9.5.x patch _without_ the above deprecation and _without_ changes in UpdatePathTestBaseFilledTest to accommodate changes in the fixture (because there aren't any), but _with_ all the other changes from the 10.x patch.

Seems good to me, leaving to quietone to RTBC if that looks good :)

@Spokje did you mean to add 9.4.0 database dumps to the patch?

@Spokje did you mean to add 9.4.0 database dumps to the patch?

Erm...not at all, uploaded the wrong patch :/
Let's retry.

Thanks for keeping me sane :)
(Or at least slightly less insane...)

So questions

1. Should test_subseven be removed
2. In ClassyUninstallUpdateTest there is still $this->assertTrue($theme_handler->themeExists('seven')); should that be removed too
3. In StableUninstallUpdateTest there is still $this->assertTrue($theme_handler->themeExists('seven')); should that be removed too
4. In UpdatePathTestBaseFilledTest seven and bartik are still there are expected themes. I assume that's true since you aren't deleting the themes but thought I'd ask

#1. Looks like this now dead code and should be removed, was previously used in core/modules/views/tests/src/Functional/UserBatchActionTest.php.

#2. #3. and #. Plan so far was to leave the upgrade path tests as-is in 9.5.x since Seven/Bartik will still be around until it's out of support (this is changed in the 10.x version already so it's just a case of not backporting those changes). So all those references are fine, at least until we find out they're not...

NW for #1 since although it's technically leftovers from a previous patch, it makes sense to remove it here.

Addressed #47.1.

@catch gotcha makes sense.

Confirmed that #47.1 is addressed in #49

Committed 1587355 and pushed to 9.5.x. Thanks!