Problem/Motivation

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

         1 core/tests/Drupal/FunctionalTests/Installer/DistributionProfileExistingSettingsTest.php
   1 core/tests/Drupal/FunctionalTests/Installer/DistributionProfileTest.php
   1 core/tests/Drupal/FunctionalTests/Installer/DistributionProfileTranslationQueryTest.php
   1 core/tests/Drupal/FunctionalTests/Installer/DistributionProfileTranslationTest.php
   1 core/tests/Drupal/FunctionalTests/Installer/InstallerTest.php
   2 core/tests/Drupal/FunctionalTests/Installer/MultipleDistributionsProfileTest.php
   1 core/tests/Drupal/FunctionalTests/Installer/StandardInstallerTest.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/FunctionalTests/Installer' | 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
#30 core-3281444-30.patch5.16 KBnod_
#22 core-3281444-22.patch6.14 KBvighneshh
#18 core-3281444-18.patch6.14 KBnod_
#16 reroll_diff_10-16.txt2.01 KBravi.shankar
#16 3281444-16.patch5.16 KBravi.shankar
#10 interdiff_5-10.txt658 bytesravi.shankar
#10 3281444-10.patch5.16 KBravi.shankar
#5 3281444-5.patch5.34 KB_shy
#2 3281444-2.patch5.37 KB_shy

Comments

deviantintegral created an issue. See original summary.

_shy’s picture

_shy
Primary language Ukrainian
Location Ukraine, Lutsk 🇺🇦
commented
StatusFileSize
new 3281444-2.patch5.37 KB

Replaced references for the Bartik and Seven themes for the provided list of classes.
Let's see if the tests pass

_shy’s picture

_shy
Primary language Ukrainian
Location Ukraine, Lutsk 🇺🇦
commented
Status: Active » Needs review

Status: Needs review » Needs work

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

_shy’s picture

_shy
Primary language Ukrainian
Location Ukraine, Lutsk 🇺🇦
commented
Status: Needs work » Needs review
StatusFileSize
new 3281444-5.patch5.34 KB

Reworked patch. Now, I hope, all tests should pass.

daffie’s picture

daffie commented
Parent issue: #3281443: Update Taxonomy tests 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 commented
Status: Needs review » Needs work

Was there a specific reason we changed the tests from using a front-end theme (bartik) to an admin theme (claro)?

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs work » Reviewed & tested by the community

I would guess that is because Claro do have a install page style whereas Olivero doesn't.

I don't think front/admin theme matters a lot here, it is mainly making sure that the theme chosen is the theme actually used.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/tests/Drupal/FunctionalTests/Installer/DistributionProfileExistingSettingsTest.php
@@ -38,7 +38,7 @@ class DistributionProfileExistingSettingsTest extends InstallerTestBase {
-          'theme' => 'bartik',
+          'theme' => 'claro',

This won't test what it should test given that Claro is the theme used for the installer. This should be either Olivero or some other theme which can be used for asserting that the theme can be changed.

ravi.shankar’s picture

ravi.shankar commented
Status: Needs work » Needs review
StatusFileSize
new 3281444-10.patch5.16 KB
new interdiff_5-10.txt658 bytes

Made changes as per comment #9, please review.

Status: Needs review » Needs work

The last submitted patch, 10: 3281444-10.patch, failed testing. View results

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs work » Postponed
Related issues: +#3295735: Fix ConfigImportUITest with olivero

Other issue with olivero needs to be fixed.

catch’s picture

catch
he/him
Primary language English
commented
Title: Update Installer tests to not use Bartik and Seven » [PP-1] Update Installer tests to not use Bartik and Seven
nod_’s picture

nod_
Primary language French
Location Lille
commented
Title: [PP-1] Update Installer tests to not use Bartik and Seven » Update Installer tests to not use Bartik and Seven
Status: Postponed » Needs review
nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Needs work
Issue tags: +Needs reroll
ravi.shankar’s picture

ravi.shankar commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new 3281444-16.patch5.16 KB
new reroll_diff_10-16.txt2.01 KB

Added reroll of patch #10 and removed needs reroll tag, please review.

Status: Needs review » Needs work

The last submitted patch, 16: 3281444-16.patch, failed testing. View results

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs work » Needs review
StatusFileSize
new core-3281444-18.patch6.14 KB

Added my "less bad" solution from #3295735-7: Fix ConfigImportUITest with olivero to make tests pass.

Status: Needs review » Needs work

The last submitted patch, 18: core-3281444-18.patch, failed testing. View results

nod_’s picture

nod_
Primary language French
Location Lille
commented

tried to find how to fix the remaining test failures in InstallerTest, no luck yet.

Help welcome :)

vighneshh’s picture

vighneshh commented
Assigned: Unassigned » vighneshh
vighneshh’s picture

vighneshh commented
StatusFileSize
new core-3281444-22.patch6.14 KB

a new patch updated test failure solution.

vighneshh’s picture

vighneshh commented
Assigned: vighneshh » Unassigned
Status: Needs work » Needs review
nod_’s picture

nod_
Primary language French
Location Lille
commented

The interdiff is 

-    $this->assertSession()->responseContains("css/components/buttons.css");
+    $this->assertSession()->responseContains("css/components/button.css");

Thanks for the find :)

nod_’s picture

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

All good thanks ! Still need to dance around Olivero not having a config at the install stage but good enough

  • catch committed a223aa1 on 10.1.x 
    Issue #3281444 by ravi.shankar, _shY, nod_, Vighneshh, deviantintegral,...
catch’s picture

catch
he/him
Primary language English
commented
Version: 10.0.x-dev » 9.5.x-dev

Committed/pushed to 10.1.x and 10.0.x, kicking off a 9.5.x test run just in case.

  • catch committed ed25429 on 10.0.x 
    Issue #3281444 by ravi.shankar, _shY, nod_, Vighneshh, deviantintegral,...
nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Needs work

Need a 9.5.x patch

nod_’s picture

nod_
Primary language French
Location Lille
commented
StatusFileSize
new core-3281444-30.patch5.16 KB

9.5.x doesn't need the olivero fix

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs work » Needs review
nod_’s picture

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

I simply removed the code I added to the patch in #18

  • catch committed cec9449 on 9.5.x 
    Issue #3281444 by ravi.shankar, nod_, _shY, Vighneshh, deviantintegral,...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 9.5.x, thanks!

Status: Fixed » Closed (fixed)

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