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 #2
deviantintegral CreditAttribution: deviantintegral at Lullabot commentedComment #3
deviantintegral CreditAttribution: deviantintegral at Lullabot commentedComment #4
_shYTried 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 55In 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
_shYComment #7
_shYSeems 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
incore/modules/breakpoint/tests/themes/breakpoint_theme_test/breakpoint_theme_test.info.yml
Comment #8
_shYIf I was right with my last guess, this patch should pass all tests.
Comment #9
_shYComment #10
deviantintegral CreditAttribution: deviantintegral at Lullabot commentedNice, this is close!
Should we change the above comment to "Install all themes used for viewing the site"?
Did you mean to change this from an admin theme (seven) to olivero?
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
_shYHi, 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
withOlivero
. Probably, will be better if here we useClaro
instead ofOlivero
as substitution of theSeven
. 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
_shYHere is a patch with the changes, described above.
Summary:
- Used
Claro
theme in admin theme cases;- Code comments changes after theme replacing;
Comment #14
_shYSeems like the first-time tests are failed, but I'm not sure why.
After retesting everything is good.
Comment #15
deviantintegral CreditAttribution: deviantintegral at Lullabot commentedExcellent. The last patch looks good to me.
Comment #16
daffie CreditAttribution: daffie commentedAssign this issue to the parent #3285205: [META] Convert test that use Bartik/Seven to Olivero/Claro.
Comment #17
quietone CreditAttribution: quietone at PreviousNext commentedThanks 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.
Setting to NW
Comment #18
quietone CreditAttribution: quietone at PreviousNext commentedI 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.
Why this change? I don't understand what 'viewing the site' means here. Why is Claro not included?
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
_shYThanks for the comments.
According to your questions:
1.
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
_shYComment #21
alexpottI think claro should be in both of these lists.
Comment #22
pooja saraah CreditAttribution: pooja saraah at Srijan | A Material+ Company for Drupal India Association commentedAttached patch against #21
Comment #23
nod_Patch looks good, #21 addressed.
Looks like patch applies to 9.5 too, so RTBC
Comment #26
lauriiiWe 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!