Problem/Motivation

There are several tests that have references to Bartik and Seven:

   1 core/tests/Drupal/Tests/Core/Asset/AssetResolverTest.php
   2 core/tests/Drupal/Tests/Core/Asset/CssCollectionGrouperUnitTest.php
   2 core/tests/Drupal/Tests/Core/Command/GenerateThemeTest.php
   3 core/tests/Drupal/Tests/Core/Extension/ExtensionDiscoveryTest.php
   5 core/tests/Drupal/Tests/Core/Extension/ThemeHandlerTest.php
   1 core/tests/Drupal/Tests/Core/Render/RendererPlaceholdersTest.php
   6 core/tests/Drupal/Tests/Core/Theme/AjaxBasePageNegotiatorTest.php

These tests should be updated to either use Olivero, Claro, or System module's test_theme so we can deprecate Bartik #3249109: Deprecate Bartik and Seven #3084814: Deprecate Seven theme as mentioned at #3278124: Convert various tests that use bartik/seven to olivero/claro.

Steps to reproduce

git grep -E '(bartik)|(seven)' -- 'core/tests/Drupal/Tests/Core' | awk -F: '{print $1}' | sort | uniq -c should return no results when this work is complete, except for \Drupal\Tests\Core\Render\RendererPlaceholdersTest.

Remaining tasks

Update the tests.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

I don't think we need release notes or a change record.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

deviantintegral created an issue. See original summary.

longwave’s picture

longwave’s picture

I used Stark or "example" where it seemed appropriate (as these will hopefully change less frequently) and Claro or Olivero otherwise.

There is one false positive in a comment in core/tests/Drupal/Tests/Core/Render/RendererPlaceholdersTest.php - this isn't referring to the theme:

328:    // Case seven: render array that has a placeholder that is:

Some other notes:

  1. +++ b/core/tests/Drupal/Tests/Core/Asset/AssetResolverTest.php
    @@ -86,9 +86,6 @@ protected function setUp(): void {
    -    $active_theme->expects($this->any())
    -      ->method('getName')
    -      ->willReturn('bartik');
         $this->themeManager->expects($this->any())
    

    The test passes without this method stub so I think we can just drop it.

  2. +++ b/core/tests/Drupal/Tests/Core/Extension/ExtensionDiscoveryTest.php
    @@ -130,13 +130,6 @@ protected function populateFilesystemStructure(array &$filesystem_structure) {
    -      'core/themes/seven/seven.info.yml' => [
    -        'type' => 'theme',
    -      ],
    -      // Override the core instance of the 'seven' theme.
    -      'sites/default/themes/seven/seven.info.yml' => [
    -        'type' => 'theme',
    -      ],
    

    This test doesn't seem to be testing what it thinks it is testing, as removing this, or breaking it in a couple of different ways, didn't cause the test to fail. This probably needs investigation and a better fix elsewhere.

deviantintegral’s picture

Status: Needs review » Needs work

The test passes without this method stub so I think we can just drop it.

Isn't that because the call to expects() is being removed too? That internally triggers an assertion where if the method is never called, it will fail the test at the end.

\Drupal\Tests\system\FunctionalJavascript\OffCanvasTestBase::getTestThemes still has seven in it?


    return ['classy', 'olivero', 'seven', 'stable', 'stark'];

On ExtensionDiscoveryTest, I agree that's worth filing a new issue for.

deviantintegral’s picture

Issue summary: View changes
nod_’s picture

Status: Needs work » Reviewed & tested by the community
Related issues: +#3298319: Update ExtensionDiscoveryTest to not use seven

for \Drupal\Tests\system\FunctionalJavascript\OffCanvasTestBase::getTestThemes it's in #3281434: Update System module tests to not use Bartik and Seven that it should be handled as per the issue summary.

Opened a follow-up for ExtensionDiscoveryTest here #3298319: Update ExtensionDiscoveryTest to not use seven

nod_’s picture

reroll

catch’s picture

Version: 10.0.x-dev » 9.5.x-dev

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

#8 doesn't apply to 9.5.x but #2 does, so I've just kicked off a test run against 9.5.x and will commit that to 9.5.x if it's green. If not we might need a re-roll.

  • catch committed fa54f53 on 10.0.x
    Issue #3281449 by longwave, nod_, deviantintegral: Update Core unit...
  • catch committed 5645eeb on 10.1.x
    Issue #3281449 by longwave, nod_, deviantintegral: Update Core unit...
nod_’s picture

looks like it's green. Quickedit test failure seems unrelated.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Yep, kicked off a retest just in case. Committed/pushed to 9.5.x, thanks!

  • catch committed c2a6b8d on 9.5.x
    Issue #3281449 by nod_, longwave, deviantintegral: Update Core unit...

Status: Fixed » Closed (fixed)

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