Problem/Motivation

Note: we moved the migration-related Color tests into their own issue here: #3301713: Update migration Color tests to not use Bartik

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

   2 core/modules/color/tests/src/Functional/ColorConfigSchemaTest.php
  15 core/modules/color/tests/src/Functional/ColorTest.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/modules/color/tests' | awk -F: '{print $1}' | sort | uniq -c should return no results when this work is complete.

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
#40 3281430-40-95x.patch1.39 KBbnjmnm
#40 interdiff_39-40.txt1003 bytesbnjmnm
#39 3281430-39-95x.patch1022 bytesbnjmnm
#26 3281430-d9.5.x-26.patch17.16 KBSpokje
#24 3281430-d10.1.x-24.patch13.5 KBSpokje
#23 Screenshot from 2022-08-04 15-12-15.png27.81 KBVighneshh

Issue fork drupal-3281430

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

deviantintegral created an issue. See original summary.

deviantintegral’s picture

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

deviantintegral CreditAttribution: deviantintegral at Lullabot commented
Title: Update Color tests to not use Bartik and Seven » Update Help Topics tests to not use Bartik and Seven
Component: color.module » help.module
Issue summary: View changes
daffie’s picture

daffie CreditAttribution: daffie commented
Parent issue: » #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.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Issue summary: View changes

The migration tests need to continue to use Bartik and/or Seven because the source site will have those themes. Also, since the Color module is deprecated these tests will move to contrib.

  • core/modules/color/tests/src/Kernel/Migrate/d7/MigrateColorTest.php - This installs bartik, which is needed to test the migration of bartik configuration. In terms of this issue, there is nothing to do here.
  • core/modules/color/tests/src/Kernel/Plugin/migrate/source/d7/ColorTest.php - No changes are needed here.

I have struck out these tests in the Issue Summary.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Issue summary: View changes

Fix formatting

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ at Acquia commented
Status: Active » Needs review

given #3270899: Remove Color module from core do we need to do this at all?

deviantintegral’s picture

deviantintegral CreditAttribution: deviantintegral at Lullabot commented

We'll need it for 9.x I think. At that point, if color hasn't been removed from 10.x yet I don't see any harm in changing it there too, given how most of these have applied easily to both branches.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ at Acquia commented
Status: Needs review » Active

Sounds good

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Issue summary: View changes

We need to do something with the color migration tests for bartik/seven.

If color, bartik, and seven are all deprecated at the same time, perhaps we could mark those tests legacy but keep them how they are. But we'd need to remove color module from core before bartik/seven in 10.0.x then to avoid things blowing up.

Alternatively, we could try moving the color tests to bartik/seven instead in order to be able to remove the themes before color modules.

I'm don't think it's useful to do an extensive refactoring of the tests to use a test color-enabled theme (unless we somehow already have one?) only to then move everything to contrib where pulling in the dependencies for testing would be fine again.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Title: Update Color tests to not use Bartik and Seven » Update Color tests to not use Bartik and Seven and/or move them to the themes

Spokje made their first commit to this issue’s fork.

Spokje opened merge request !2583

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ at Acquia commented
Status: Active » Needs review

Way easier than I expected :)

Spokje’s picture

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

1 out of 4 tests does not a summer make ;)

Spokje’s picture

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

All tests now run on color_test_theme, which leaves us with:

I have no clue what to do with #5 and especially:

core/modules/color/tests/src/Kernel/Migrate/d7/MigrateColorTest.php - This installs bartik, which is needed to test the migration of bartik configuration. In terms of this issue, there is nothing to do here.

We _can_ move it to Bartik, but that would be a problem when color goes out of core first.
We _can_ split that off into a separate issue and review/commit what we got now, it at least decouples color from bartik a bit more.

Putting this on NR to get more opinions on how to proceed.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

Let's split the migration test out, I'm not sure what to do with it either. We might to do something like mark it skipped and then re-enable in the contrib versions.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje as a volunteer commented
Title: Update Color tests to not use Bartik and Seven and/or move them to the themes » Update non-migration Color tests to not use Bartik and Seven and/or move them to the themes
Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje as a volunteer commented
Issue summary: View changes

Migration related test now has its own issue: #3301713: Update migration Color tests to not use Bartik

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje as a volunteer commented
Related issues: +#3301713: Update migration Color tests to not use Bartik
Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje as a volunteer commented
Title: Update non-migration Color tests to not use Bartik and Seven and/or move them to the themes » Update non-migration Color tests to not use Bartik
Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje as a volunteer commented

The current MR definitely needs a separate 9.5.x patch, I _think_ the MR will apply to 10.1.x, but let's get somebody to review the MR first before getting into that grind.

Vighneshh’s picture

Vighneshh CreditAttribution: Vighneshh at QED42 for Drupal India Association commented
Status: Needs review » Reviewed & tested by the community
FileSize
Screenshot from 2022-08-04 15-12-15.png27.81 KB

Tested The current MR and found no Non-migration errors
Adding screenshot of the command output
Moving to RTBC.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje as a volunteer commented
FileSize
3281430-d10.1.x-24.patch13.5 KB
Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje as a volunteer commented
Version: 10.0.x-dev » 9.5.x-dev
Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje as a volunteer commented
FileSize
3281430-d9.5.x-26.patch17.16 KB
Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje as a volunteer commented

MR !2583 applies to both 10.0.x and 10.1.x.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje as a volunteer commented
Assigned: Spokje » Unassigned

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 26: 3281430-d9.5.x-26.patch, failed testing. View results

Spokje’s picture

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

(Rather weird) random test-failure, did a retest, all green => back to RTBC

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje as a volunteer commented
Status: Needs review » Reviewed & tested by the community
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Reviewed & tested by the community » Needs review

Moving to needs review because #23 doesn't make it clear whether the code changes have been reviewed.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje as a volunteer commented
nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ at Acquia commented
Status: Needs review » Reviewed & tested by the community

Looks good to me, better than before :)

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 10.1.x, cherry-picked to 10.0.x and 9.5.x, thanks!

  • catch committed 861205a on 10.0.x 
    Issue #3281430 by Spokje, deviantintegral, lauriii: Update non-migration...
  • catch committed 8d98907 on 10.1.x
    Issue #3281430 by Spokje, deviantintegral, lauriii: Update non-migration...
  • catch committed e39d894 on 9.5.x
    Issue #3281430 by Spokje, deviantintegral, lauriii: Update non-migration...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Issue summary: View changes

Spokje closed merge request !2583

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
FileSize
3281430-39-95x.patch1022 bytes

This potentially fixes the PHP 7.3 conflict introduced in this issue. Assumes HEAD still has the issue and hasn't been reverted.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
FileSize
interdiff_39-40.txt1003 bytes
3281430-40-95x.patch1.39 KB

The fix in #39 meant 2 unused USE statements.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje as a volunteer commented

Thanks @bnjmnm.

I originally had a 9.5.x patch in #26, but after the back to NR and the short time between RTBC and Fixed and didn't had the time to create a new one.

This is indeed needed for PHP 7.3.
RTBC + 1 if TestBot agrees (which it should).

  • xjm committed 97dfc92 on 9.5.x 
    Issue #3281430 followup by bnjmnm: Fix tests on PHP 7.3.
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

Fail is a known random unrelated to Color.

Committed the hotfix to 9.5.x. Thanks @bnjmnm for the quick followup!

Status: Fixed » Closed (fixed)

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