Problem/Motivation

These two core tests should be moved to their respective themes, instead of being part of Drupal core tests, 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.

      3 core/tests/Drupal/FunctionalTests/Theme/SevenLayoutBuilderTest.php
   5 core/tests/Drupal/FunctionalTests/Theme/BartikTest.php

Also there are themetests for claro and olivero that use seven and bartik to test uninstallation. These should use a different theme.

./Theme/ClaroTest.php:    $this->cssSelect('a[title="Install Seven as default theme"]')[0]->click();
./Theme/OliveroTest.php:    $this->cssSelect('a[title="Install Bartik as default theme"]')[0]->click();

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
#6 3281457-6.patch3.03 KBquietone
#6 interdiff-3-6.txt1.56 KBquietone
#3 3281457.patch1.21 KBcatch
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

deviantintegral created an issue. See original summary.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Title: Move Seven and Bartik tests to their respective themes » Move Seven and Bartik tests in the core FunctionalTests namespace to their respective themes
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Active » Needs review
FileSize
3281457.patch1.21 KB

Hopefully all we need to do for this one.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Title: Move Seven and Bartik tests in the core FunctionalTests namespace to their respective themes » Move Seven and Bartik tests in the core FunctionalTests\Theme namespace to their respective themes
Status: Needs review » Needs work

This does move the two files but ...

I applied the patch and I searched core/tests/Drupal/FunctionalTests for other instances of bartik and seven. There are some in an Installer directory which are being done in #3281444: Update Installer tests to not use Bartik and Seven. Therefor, changing the namespace in the title.

There is also 

./Theme/ClaroTest.php:    $this->cssSelect('a[title="Install Seven as default theme"]')[0]->click();
./Theme/OliveroTest.php:    $this->cssSelect('a[title="Install Bartik as default theme"]')[0]->click();

What about those?

bbrala’s picture

bbrala
Location Netherlands
CreditAttribution: bbrala as a volunteer and at SWIS commented

Well, i was in the same process as you. First dug into the parent issues regarding this to see why only these. Then applied the patch, which looks good and ended up at the same question. There is a few references left, and while looking to see if things are covered in #3278124: Convert various tests that use bartik/seven to olivero/claro it seems the ones in claro and olivero (see #4) are not mentioned in the parent issue.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Status: Needs work » Needs review
FileSize
interdiff-3-6.txt1.56 KB
3281457-6.patch3.03 KB

What about using a test theme for testing the uninstall? There was a @todo in ClaroTest to removed the uninstall test. However, I left it in because there is the same test in Olivero.

bbrala’s picture

bbrala
Location Netherlands
CreditAttribution: bbrala as a volunteer and at SWIS commented
Issue summary: View changes

I think your approach is good. Bit of scope creep though for this issue, so IS needs updating. I'll do that.

Change is looking good. If tests are green then i'll RTBC.

bbrala’s picture

bbrala
Location Netherlands
CreditAttribution: bbrala as a volunteer and at SWIS commented
Status: Needs review » Reviewed & tested by the community

As i post, the tests were done.

catch’s picture

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

#6 looks good to me too, good spot on installing Seven in the test.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 6: 3281457-6.patch, failed testing. View results

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Needs work » Reviewed & tested by the community

Don't think the ckeditor failures were related.

  • catch committed 5c444d0 on 10.1.x 
    Issue #3281457 by quietone, catch, bbrala, deviantintegral: Move Seven...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 10.0.x-dev » 9.5.x-dev
Status: Reviewed & tested by the community » Fixed

I did an initial version of the patch here, but the changes were just the file moves without any of the substantive changes, so I think I'm still clear to commit this one.

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

  • catch committed b2d8a8f on 10.0.x 
    Issue #3281457 by quietone, catch, bbrala, deviantintegral: Move Seven...
  • catch committed 9b5ead2 on 9.5.x
    Issue #3281457 by quietone, catch, bbrala, deviantintegral: Move Seven...

Status: Fixed » Closed (fixed)

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