Update various module tests to not use Bartik and Seven [#3281454]

Problem/Motivation

There are several references to Bartik and Seven across various modules:

   1 core/modules/breakpoint/tests/src/Kernel/BreakpointDiscoveryTest.php
   1 core/modules/comment/tests/src/Functional/CommentDisplayConfigurableTest.php
   1 core/modules/editor/tests/src/Kernel/EditorImageDialogTest.php
   1 core/modules/search/tests/src/Functional/SearchAdminThemeTest.php
   1 core/modules/views/tests/src/Functional/UserBatchActionTest.php
   2 core/modules/responsive_image/tests/src/Unit/ResponsiveImageStyleConfigEntityUnitTest.php
   2 core/modules/views_ui/tests/src/Functional/ViewEditTest.php
   2 core/modules/ckeditor5/tests/src/Functional/AddedStylesheetsTest.php
   3 core/modules/jsonapi/tests/src/Functional/TermTest.php
   4 core/modules/menu_link_content/tests/src/Functional/MenuLinkContentTranslationUITest.php
   4 core/modules/node/tests/src/Functional/NodeTranslationUITest.php
   3 core/modules/block_content/tests/src/Functional/BlockContentTypeTest.php
   1 core/modules/node/tests/src/Functional/NodeDisplayConfigurableTest.php
   1 core/modules/views/tests/src/Kernel/Plugin/DisplayPageTest.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.

I've combined these into one issue as they didn't make the cut for my initial pass of complexity and number of references.

Steps to reproduce

git grep -E '(bartik)|(seven)' -- core/modules/breakpoint/tests/src/Kernel/BreakpointDiscoveryTest.php \
core/modules/comment/tests/src/Functional/CommentDisplayConfigurableTest.php \
core/modules/editor/tests/src/Kernel/EditorImageDialogTest.php \
core/modules/search/tests/src/Functional/SearchAdminThemeTest.php \
core/modules/views/tests/src/Functional/UserBatchActionTest.php \
core/modules/responsive_image/tests/src/Unit/ResponsiveImageStyleConfigEntityUnitTest.php \
core/modules/views_ui/tests/src/Functional/ViewEditTest.php \
core/modules/ckeditor5/tests/src/Functional/AddedStylesheetsTest.php \
core/modules/jsonapi/tests/src/Functional/TermTest.php \
core/modules/menu_link_content/tests/src/Functional/MenuLinkContentTranslationUITest.php \
core/modules/node/tests/src/Functional/NodeTranslationUITest.php \
core/modules/block_content/tests/src/Functional/BlockContentTypeTest.php \
core/modules/node/tests/src/Functional/NodeDisplayConfigurableTest.php \
core/modules/views/tests/src/Kernel/Plugin/DisplayPageTest.php \
| awk -F: '{print $1}' | sort | uniq -c

should return no results when this work is complete.

If any of these test updates turn out to be complex, we should split them off into a separate issue as it's unlikely there's any dependencies or shared code between these tests.

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.

Comment	File	Size	Author
#22	3281454-22.patch	12.56 KB	pooja saraah
#19	3281454-19.patch	12.79 KB	_shy
#12	3281454-12.patch	12.83 KB	_shy
#8	3281454-8.patch	12.75 KB	_shy
#4	3281454.patch	12.18 KB	_shy

Comments

Comment #1

19 May 2022 at 19:05

deviantintegral created an issue. See original summary.

Comment #2

deviantintegral commented 27 May 2022 at 20:22

Issue summary:

View changes

Comment #3

deviantintegral commented 27 May 2022 at 20:26

Issue summary:

View changes

Comment #4

_shy

Ukrainian

Ukraine, Lutsk 🇺🇦

commented 28 May 2022 at 09:17

Status	File	Size
new	3281454.patch	12.18 KB

Tried to replace Seven/Bartik themes with the Olivero/Claro for provided files from the task description.
Also, want to notice that some files use, for example, the word "seven" in a number context.
core/modules/views_ui/tests/src/Functional/ViewEditTest.php line 55

// Test that a long administrative comment is truncated.
    $edit = ['display_comment' => 'one two three four five six seven eight nine ten eleven twelve thirteen fourteen fifteen'];

In our case, I changed themes in all files (from the provided list) except the following:
core/modules/views_ui/tests/src/Functional/ViewEditTest.php
core/modules/jsonapi/tests/src/Functional/TermTest.php

Hope, the patch passes all tests.

Comment #5

_shy

Ukrainian

Ukraine, Lutsk 🇺🇦

commented 28 May 2022 at 11:04

Status:

Active

» Needs review

Comment #6

28 May 2022 at 11:21

Status:

Needs review

» Needs work

The last submitted patch, 4: 3281454.patch, failed testing. View results

Comment #7

_shy

Ukrainian

Ukraine, Lutsk 🇺🇦

commented 28 May 2022 at 11:47

Seems like failed just one test.
Probably i know what the problem. They try to assert array with their test theme, but there by default set base theme: bartik in core/modules/breakpoint/tests/themes/breakpoint_theme_test/breakpoint_theme_test.info.yml

Comment #8

_shy

Ukrainian

Ukraine, Lutsk 🇺🇦

commented 28 May 2022 at 14:14

Status	File	Size
new	3281454-8.patch	12.75 KB

If I was right with my last guess, this patch should pass all tests.

Comment #9

_shy

Ukrainian

Ukraine, Lutsk 🇺🇦

commented 28 May 2022 at 15:20

Status:

Needs work

» Needs review

Comment #10

deviantintegral commented 29 May 2022 at 20:05

Status:

Needs review

» Needs work

Nice, this is close!

+++ b/core/modules/block_content/tests/src/Functional/BlockContentTypeTest.php
@@ -204,7 +204,7 @@ class BlockContentTypeTest extends BlockContentTestBase {
+    $themes = ['olivero', 'stark'];

Should we change the above comment to "Install all themes used for viewing the site"?

+++ b/core/modules/ckeditor5/tests/src/Functional/AddedStylesheetsTest.php
@@ -81,8 +81,8 @@ class AddedStylesheetsTest extends BrowserTestBase {
+    $theme_installer->install(['test_ckeditor_stylesheets_relative', 'olivero']);
+    $this->config('system.theme')->set('admin', 'olivero')->save();
     $this->config('node.settings')->set('use_admin_theme', TRUE)->save();

Did you mean to change this from an admin theme (seven) to olivero?

+++ b/core/modules/search/tests/src/Functional/SearchAdminThemeTest.php
@@ -30,7 +30,7 @@ class SearchAdminThemeTest extends BrowserTestBase {

Should this include claro? This looks like a functional test for editing users, which I think would usually be in an admin theme?

Comment #11

_shy

Ukrainian

Ukraine, Lutsk 🇺🇦

commented 30 May 2022 at 15:09

Hi, deviantintegral.
Thanks for your time.

1. I think yes. Your variant is good enough descriptive. I can change it in the next patch.
2. Yeah, seems like I replace the theme Seven with Olivero. Probably, will be better if here we use Claro instead of Olivero as substitution of the Seven. What you think about it?
3. Yes, I think you are right. Claro theme used as admin theme as usual.

Actually, I don't know why I use the Olivero theme in these cases, probably miss something.
Thank you.

Comment #12

_shy

Ukrainian

Ukraine, Lutsk 🇺🇦

commented 31 May 2022 at 05:20

Status:

Needs work

» Needs review

Status	File	Size
new	3281454-12.patch	12.83 KB

Here is a patch with the changes, described above.
Summary:
- Used Claro theme in admin theme cases;
- Code comments changes after theme replacing;

Comment #13

31 May 2022 at 06:05

Status:

Needs review

» Needs work

The last submitted patch, 12: 3281454-12.patch, failed testing. View results

Comment #14

_shy

Ukrainian

Ukraine, Lutsk 🇺🇦

commented 31 May 2022 at 07:25

Status:

Needs work

» Needs review

Seems like the first-time tests are failed, but I'm not sure why.
After retesting everything is good.

Comment #15

deviantintegral commented 6 June 2022 at 19:42

Status:

Needs review

» Reviewed & tested by the community

Excellent. The last patch looks good to me.

Comment #16

daffie commented 11 June 2022 at 09:48

Parent issue:

#3281452: Update Core kernel 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.

Comment #17

quietone commented 12 June 2022 at 00:51

Status:

Reviewed & tested by the community

» Needs work

Thanks for working on this.

I applied the patch locally, ran the grep given in the IS and two file were found. I did not review the changes.

      3 core/modules/jsonapi/tests/src/Functional/TermTest.php
      2 core/modules/views_ui/tests/src/Functional/ViewEditTest.php

Setting to NW

Comment #18

quietone commented 12 June 2022 at 01:01

I take that back. The files are using the word 'seven' but not in the context of a theme.

So I looked at the patch and have some questions.

+++ b/core/modules/block_content/tests/src/Functional/BlockContentTypeTest.php
@@ -203,8 +203,8 @@ class BlockContentTypeTest extends BlockContentTestBase {
+    // Install all themes used for viewing the site.

Why this change? I don't understand what 'viewing the site' means here. Why is Claro not included?

+++ b/core/modules/menu_link_content/tests/src/Functional/MenuLinkContentTranslationUITest.php
@@ -92,18 +92,18 @@ class MenuLinkContentTranslationUITest extends ContentTranslationUITestBase {
+    // Set up Claro as the admin theme to test.

+++ b/core/modules/node/tests/src/Functional/NodeTranslationUITest.php
@@ -252,16 +252,16 @@ class NodeTranslationUITest extends ContentTranslationUITestBase {
+    // Set up Claro as the admin theme and use it for node editing.

Can't we make the comment generic so we can skip future changes? Something like use the 'default admin theme' or 'default theme'?

Comment #19

_shy

Ukrainian

Ukraine, Lutsk 🇺🇦

commented 13 June 2022 at 05:57

Status	File	Size
new	3281454-19.patch	12.79 KB

Thanks for the comments.

According to your questions:
1.

Why this change? I don't understand what 'viewing the site' means here. Why is Claro not included?

First time, when I check this test, I thought that there should be preferred non-admin themes. That's why I did not include Claro, because usually, this is the admin theme. That's why @deviantintegral suggested I change the description. But after double checking, I realized I had made a mistake. It's wrong to not add the Claro theme here, so the description should stay as it has been.

2. Yeah, you are right, I made adjustments to the description.

Here is a patch with changes described above.

Comment #20

_shy

Ukrainian

Ukraine, Lutsk 🇺🇦

commented 13 June 2022 at 07:10

Status:

Needs work

» Needs review

Comment #21

alexpott

he/they

English

🇪🇺🌍

commented 13 June 2022 at 09:18

+++ b/core/modules/views/tests/src/Functional/UserBatchActionTest.php
@@ -34,7 +34,7 @@ class UserBatchActionTest extends BrowserTestBase {
-    $themes = ['bartik', 'classy', 'olivero', 'seven', 'test_subseven'];
+    $themes = ['classy', 'olivero'];

+++ b/core/modules/views/tests/src/Kernel/Plugin/DisplayPageTest.php
@@ -244,7 +244,7 @@ class DisplayPageTest extends ViewsKernelTestBase {
-    $themes = ['bartik', 'classy', 'olivero', 'seven', 'stable', 'stark'];
+    $themes = ['classy', 'olivero', 'stable', 'stark'];

I think claro should be in both of these lists.

Comment #22

pooja saraah commented 15 June 2022 at 13:18

Status	File	Size
new	3281454-22.patch	12.56 KB

Attached patch against #21

Comment #23

nod_

French

Lille

commented 13 July 2022 at 07:15

Status:

Needs review

» Reviewed & tested by the community

Patch looks good, #21 addressed.

Looks like patch applies to 9.5 too, so RTBC

Comment #24

13 July 2022 at 09:02

lauriii committed cd700d8 on 10.1.x

Issue #3281454 by _shY, pooja saraah, deviantintegral, quietone,...

Comment #25

13 July 2022 at 09:02

lauriii committed 8c03eb7 on 10.0.x

Issue #3281454 by _shY, pooja saraah, deviantintegral, quietone,...

Comment #26

lauriii

he/him

Finnish

Finland

commented 13 July 2022 at 09:02

Version:	10.0.x-dev	» 9.5.x-dev
Status:	Reviewed & tested by the community	» Fixed

+++ b/core/modules/views/tests/src/Functional/UserBatchActionTest.php
@@ -34,7 +34,7 @@ class UserBatchActionTest extends BrowserTestBase {
-    $themes = ['bartik', 'classy', 'olivero', 'seven', 'test_subseven'];
+    $themes = ['classy', 'olivero', 'claro'];

We effectively lose test coverage as part of this since we are no longer testing any subthemes. I think this was specifically testing a subtheme of Seven and since we don't officially support subtheming Claro (and we know it's somewhat broken), it seems fine to leave this out.

Committed cd700d8 and pushed to 10.1.x. Also cherry-picked to 10.0.x and 9.5.x. Thanks!

Comment #27

13 July 2022 at 09:02

lauriii committed 3668a96 on 9.5.x

Issue #3281454 by _shY, pooja saraah, deviantintegral, quietone,...

Comment #28

27 July 2022 at 09:04

Status:

Fixed

» Closed (fixed)

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

Update various module tests to not use Bartik and Seven

Problem/Motivation

Steps to reproduce

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Comments

Parent issue

Child issues

Related issues