Problem/Motivation

There are several references to Bartik and Seven:

   2 core/tests/Drupal/KernelTests/Core/Asset/ResolvedLibraryDefinitionsFilesMatchTest.php
   1 core/tests/Drupal/KernelTests/Core/Config/ConfigImporterTest.php
   1 core/tests/Drupal/KernelTests/Core/Config/DefaultConfigTest.php
   4 core/tests/Drupal/KernelTests/Core/Theme/ConfirmClassyCopiesTest.php
   3 core/tests/Drupal/KernelTests/Core/Theme/ThemeInstallerTest.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/KernelTests/Core/' | awk -F: '{print $1}' | sort | uniq -c should return no results when this work is complete, except for ResolvedLibraryDefinitionsFilesMatchTest and ConfirmClassyCopiesTest.

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.

CommentFileSizeAuthor
#10 3281452-10.patch6.11 KBmrinalini9
#2 3281452-2.patch6.11 KBlongwave
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

deviantintegral created an issue. See original summary.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Active » Needs review
FileSize
3281452-2.patch6.11 KB

These references must remain:

core/tests/Drupal/KernelTests/Core/Asset/ResolvedLibraryDefinitionsFilesMatchTest.php
63:    'bartik',
66:    'seven',

core/tests/Drupal/KernelTests/Core/Theme/ConfirmClassyCopiesTest.php
366:      'seven' => [
367:        'theme-name' => 'seven',
511:      // Will be populated when Classy libraries are copied to Bartik.
512:      'bartik' => [
513:        'theme-name' => 'bartik',

ResolvedLibraryDefinitionsFilesMatchTest tests all core themes via a hardcoded list. ConfirmClassyCopiesTest ensures templates are correctly copied. Both tests need to stay as-is until the themes are removed.

DefaultConfigTest manually installed Seven before, but the test passes without it. There is a comment that says "If a theme provides default configuration but does not have a schema then this test needs to enable it" but Seven has a config schema so I guess this is no longer needed.

deviantintegral’s picture

deviantintegral CreditAttribution: deviantintegral at Lullabot commented

You're finding some nice test debt! 🙌

I'm going to update the issue to mark the exceptions you found. Otherwise, the diff looks good to me.

deviantintegral’s picture

deviantintegral CreditAttribution: deviantintegral at Lullabot commented
Issue summary: View changes
deviantintegral’s picture

deviantintegral CreditAttribution: deviantintegral at Lullabot commented
Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: 3281452-2.patch, failed testing. View results

daffie’s picture

daffie CreditAttribution: daffie commented
Parent issue: #3281451: Update Ajax FunctionalJavascript test to not use Bartik and Seven » #3285205: [META] Convert test that use Bartik/Seven to Olivero/Claro

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

deviantintegral’s picture

deviantintegral CreditAttribution: deviantintegral at Lullabot commented
Status: Needs work » Needs review

I'm skeptical about the test failures unless we have some surprising cross-test dependencies. Requeued!

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje as a volunteer commented
Status: Needs review » Needs work

This one needs a 9.4.x/9.5.x patch currently, otherwise looks grand.

mrinalini9’s picture

mrinalini9 CreditAttribution: mrinalini9 at Srijan | A Material+ Company for Drupal India Association commented
Status: Needs work » Needs review
FileSize
3281452-10.patch6.11 KB

Rerolled patch #2 for the 9.5.x branch, please review it.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje as a volunteer commented
Version: 10.0.x-dev » 9.4.x-dev
Status: Needs review » Reviewed & tested by the community

LGTM, changed version to 9.4.x-dev, since related issues were all backported as far as that branch because it being a "test only change".

  • lauriii committed 4411770 on 10.1.x 
    Issue #3281452 by longwave, mrinalini9, deviantintegral, Spokje: Update...

  • lauriii committed 5396462 on 10.0.x 
    Issue #3281452 by longwave, mrinalini9, deviantintegral, Spokje: Update...

  • lauriii committed 5904727 on 9.5.x 
    Issue #3281452 by longwave, mrinalini9, deviantintegral, Spokje: Update...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Version: 9.4.x-dev » 9.5.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 4411770 and pushed to 10.1.x. Cherry-picked to 10.0.x and 9.5.x. Thanks!

Status: Fixed » Closed (fixed)

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