Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Because of #3083275: [meta] Update tests that rely on Classy to not rely on it anymore and Classy being deprecated in Drupal 9 + removed in Drupal 10,: Tests that aren't specifically testing Classy yet declare $defaultTheme = 'classy';
should be refactored to use Stark as the default theme instead.
Proposed resolution
Change all tests in this module to use Stark as the default theme, and refactor the tests where needed so they continue to function properly.
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#35 | reroll_diff_3112547-24_3112547-33.txt | 3.6 KB | yogeshmpawar |
#33 | 3112547-33.patch | 13.69 KB | yogeshmpawar |
#24 | 3112547-24.patch | 13.75 KB | andypost |
#24 | interdiff.txt | 667 bytes | andypost |
Comments
Comment #2
bnjmnmComment #3
joachim CreditAttribution: joachim commentedThis one is being changed to stable rather than stark; is that intended?
Comment #4
LendudeThis addresses #3, it can use stark but needs a little work.
Comment #8
andypostall of them could use parent's test default value
Comment #9
dww#4 no longer applies to 9.3.x. Working on a reroll now. Then I can address #8.
Comment #10
andypostThere's
core/modules/views_ui/tests/themes/views_test_classy_subtheme
which depends on classy#8 could use follow-up as not clear how setting default theme in abstract classes will affect it
Comment #11
dwwOoof, that wasn't trivial. I'm out of time for the night, so I didn't deal with #8. But I think this is properly merged with changes in 9* since #4.
Interdiff is confused, so here's a raw diff of the two patch files (FWIW).
core/modules/views_ui/tests/src/Functional/PreviewTest.php
is now failing locally in unexplainable ways. ;) Let's see what the bot thinks.Comment #12
andypostwhy this been added?
First argument should be expected value
Comment #13
dwwThanks for review, @andypost. Re #12:
Comment #14
andypostHere's fix for
\Drupal\Tests\views_ui\Functional\PreviewTest::testPreviewUI()
+ a bit of clean-up for readabilityComment #17
LendudeLooks great, just one little thing I'm wondering:
The fact that the current page gets an 'is-active' class seems like relevant test coverage that we are losing here. Is there other coverage for this? Or are we comfortable removing this?
Comment #18
andypost@Lendude nice catch! active link class should be tested separate place, OTOH views using own pager implementations and would be great to make sure the class is set (IIRC it is set by JS and that's why test require browser)
Comment #19
andypostbtw What's about test theme which depends on classy? - asked in #10
Comment #20
andypostthere's no class attached to element because it's not a link (this assertion left)
The comment above said nothing about active class, not sure it should be attached to non-links
Comment #21
andypostLet's see what will fail, the theme could get rename
Comment #22
andypostand as theme description tells
Comment #24
andypostI see no reason to rename, just set theme to Stark
Comment #25
danflanagan8I agree. As far as I can tell, the
is-active
class only gets added to the pager by classy, not by stark or the pager template in views.The first "Navigate to next page" comment is not accurate. It looks like maybe it was accidentally copied from below?
Other than that comment, I think this patch looks great. Back to NW to fix that comment.
Comment #26
danflanagan8Well, one more thing...
The variable name
$view_description
is misleading because this is really finding the View name. I know this bad variable name is already present. But since we change all three lines that refer to the variable (there are two more later in the class), can we change the variable name to$view_name
?Or is that still out of scope?
Comment #28
mglamanI'd consider it out of scope, since we're focused on just replacing CSS selectors and XPath selectors.
Probably. But fixing these things is probably out of scope for this issue.
Going to kick it back to Needs Review on that, since the NW was on possibly out of scope items.
I have a few nits on the specificity in selectors:
Do we need
strong
? Isn't.views-ui-view-name
enough?Ditto – text should return fine without specifying
strong
Comment #29
mglamanForgot to change the status.
Comment #30
danflanagan8I insist that
is in scope since it's being added in the patch.
But I'm convinced that changing the misleading variable name (see #26) is out of scope.
I could go either way on the
strong
tags. Thestrong
I think is trying to imitate the specificity provided by theh3
, though it's likely that neither theh3
nor thestrong
is needed.Comment #31
mglamanOops, I didn't read correctly :) Looks like a bit of copy paste, comment does need to be removed/tweaked.
I'm used to trying to decouple tests from strict markup structure. But it's the Views UI. It's probably best to be super specific here to catch changes.
Comment #32
danflanagan8Patch no longer applies to 9.4.x
Comment #33
yogeshmpawarReroll against 9.4.x branch & addressed #31
Comment #34
danflanagan8@yogeshmpawar, thanks for the re-roll. Reviewing is easier if you add a re-roll diff per these instructions:
https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa...
I think it's also helpful to do the re-roll separately from additional changes so that the additional changes can have a real interdiff.
That said, I made my own diffs and the new patch looks like a correct re-roll with the change requested in #31.
Still needs more thorough review.
Comment #35
yogeshmpawarThanks @danflanagan8.
As per given reference given in #34 I have added an interdiff.
Comment #36
danflanagan8I'm happy enough with this.
I applied the patch and looked through all the assertions that used css or xpath in the modified tests. Almost all the modified selectors are used in positive assertions. The few negative assertions have positive counterparts. This means we aren't likely to accidentally lose test coverage. The one notable exception I found was in
FieldUITest::testFieldUI
:The xpath selector is unchanged in the patch and it is used in a negative assertion with no corresponding positive assertion. In fact, there appear to be no other tests related to this "More" form element. That said, this selector has nothing to do with Classy so we're not losing coverage by changing to Stark.
I'm going to RTBC this.
Comment #37
mglaman+1 to RTBC
Comment #38
danflanagan8Updating priority to Major just like @xjm did for #3248295: Taxonomy tests should not rely on Classy and adding D10 tag.
Comment #39
alexpottCommitted and pushed 38ad49db1d to 10.0.x and 63b32ac5c6 to 9.4.x and 529d4a64b0 to 9.3.x. Thanks!
Backported to 9.3.x to keep the tests aligned.