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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bnjmnm created an issue. See original summary.

bnjmnm’s picture

Assigned: bnjmnm » Unassigned
Status: Active » Needs review
FileSize
12.12 KB
joachim’s picture

+++ b/core/modules/views_ui/tests/src/FunctionalJavascript/ViewsListingTest.php
@@ -20,7 +20,7 @@ class ViewsListingTest extends WebDriverTestBase {
-  protected $defaultTheme = 'classy';
+  protected $defaultTheme = 'stable';
 

This one is being changed to stable rather than stark; is that intended?

Lendude’s picture

This addresses #3, it can use stark but needs a little work.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

andypost’s picture

+++ b/core/modules/views_ui/tests/src/Functional/FieldUITest.php
@@ -16,7 +16,7 @@ class FieldUITest extends UITestBase {
-  protected $defaultTheme = 'classy';
+  protected $defaultTheme = 'stark';

+++ b/core/modules/views_ui/tests/src/Functional/PreviewTest.php
@@ -19,7 +19,7 @@ class PreviewTest extends UITestBase {
-  protected $defaultTheme = 'classy';
+  protected $defaultTheme = 'stark';

+++ b/core/modules/views_ui/tests/src/FunctionalJavascript/DisplayTest.php
@@ -31,7 +31,7 @@ class DisplayTest extends WebDriverTestBase {
-  protected $defaultTheme = 'classy';
+  protected $defaultTheme = 'stark';

+++ b/core/modules/views_ui/tests/src/FunctionalJavascript/PreviewTest.php
@@ -34,7 +34,7 @@ class PreviewTest extends WebDriverTestBase {
-  protected $defaultTheme = 'classy';
+  protected $defaultTheme = 'stark';

+++ b/core/modules/views_ui/tests/src/FunctionalJavascript/ViewsListingTest.php
@@ -20,7 +20,7 @@ class ViewsListingTest extends WebDriverTestBase {
-  protected $defaultTheme = 'classy';
+  protected $defaultTheme = 'stark';

all of them could use parent's test default value

dww’s picture

Assigned: Unassigned » dww
Status: Needs review » Needs work
Issue tags: +Needs reroll

#4 no longer applies to 9.3.x. Working on a reroll now. Then I can address #8.

andypost’s picture

There'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

dww’s picture

Assigned: dww » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
13.58 KB
11.06 KB

Ooof, 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.

andypost’s picture

Assigned: Unassigned » dww
Status: Needs review » Needs work
  1. +++ b/core/modules/views_ui/tests/src/Functional/PreviewTest.php
    @@ -97,6 +98,7 @@ public function testPreviewUI() {
         $settings->set('ui.show.sql_query.enabled', TRUE)->save();
    +    $settings->set('ui.show.sql_query.where', 'above')->save();
         $this->submitForm($edit = ['view_args' => '100'], 'Update preview');
    

    why this been added?

  2. +++ b/core/modules/views_ui/tests/src/FunctionalJavascript/PreviewTest.php
    @@ -197,14 +197,16 @@ public function testPreviewWithPagersUI() {
    +    $this->assertEquals(trim($elements[0]->getHtml()), 'Page 1', 'Element for current page is not a link.');
    
    @@ -213,14 +215,17 @@ public function testPreviewWithPagersUI() {
    +    $this->assertEquals($previous_page_link_title, 'Go to previous page');
    ...
    +    $this->assertEquals($next_page_link_title, 'Go to next page');
    

    First argument should be expected value

dww’s picture

Assigned: dww » Unassigned
Status: Needs work » Needs review
FileSize
13.16 KB
2.72 KB

Thanks for review, @andypost. Re #12:

  1. Because I was trying to get that test passing again locally, and I thought the default for that setting might have changed and that we needed to explicitly set it to 'above' before we assert the query is above the preview. But I don't think that was my problem, and I forgot to re-remove it.
  2. Right, good point. I was just converting from assertEqual() to assertEquals(), but forgot to flip the params. Fixed.
andypost’s picture

Here's fix for \Drupal\Tests\views_ui\Functional\PreviewTest::testPreviewUI() + a bit of clean-up for readability

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

The last submitted patch, 13: 3112547-13.patch, failed testing. View results

Lendude’s picture

Looks great, just one little thing I'm wondering:

+++ b/core/modules/views_ui/tests/src/FunctionalJavascript/PreviewTest.php
@@ -213,14 +214,15 @@ public function testPreviewWithPagersUI() {
-    $this->assertClass($elements[1], 'is-active', 'Element for current page has .is-active class.');
-    $this->assertEmpty($elements[1]->find('css', 'a'), 'Element for current page has no link.');
+    $this->assertEquals('Page 2', trim($elements[1]->getHtml()), 'Element for current page is not a link.');

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?

andypost’s picture

Status: Needs review » Needs work

@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)

andypost’s picture

btw What's about test theme which depends on classy? - asked in #10

andypost’s picture

Status: Needs work » Needs review

there's no class attached to element because it's not a link (this assertion left)

+++ b/core/modules/views_ui/tests/src/FunctionalJavascript/PreviewTest.php
@@ -197,14 +197,15 @@ public function testPreviewWithPagersUI() {
     // Verify elements and links to pages.
     // We expect to find current pages element with no link, next page element
     // with a link, and not to find previous page element.
-    $this->assertClass($elements[0], 'is-active', 'Element for current page has .is-active class.');
+    $this->assertEquals('Page 1', trim($elements[0]->getHtml()), 'Element for current page is not a link.');

The comment above said nothing about active class, not sure it should be attached to non-links

andypost’s picture

Let's see what will fail, the theme could get rename

andypost’s picture

FileSize
668 bytes
13.76 KB

and as theme description tells

Status: Needs review » Needs work

The last submitted patch, 22: 3112547-22.patch, failed testing. View results

andypost’s picture

I see no reason to rename, just set theme to Stark

danflanagan8’s picture

Status: Needs review » Needs work

active link class should be tested separate place

I 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.

+++ b/core/modules/views_ui/tests/src/FunctionalJavascript/PreviewTest.php
@@ -197,14 +197,15 @@ public function testPreviewWithPagersUI() {
+    // Navigate to next page.
+    $next_page_link = $elements[1]->find('css', 'a');
+    $this->assertNotEmpty($next_page_link, 'Link to next page found.');
+    $this->assertEquals('Go to next page', $next_page_link->getAttribute('title'));
 
     // Navigate to next page.
-    $element = $this->assertSession()->elementExists('xpath', '//li[contains(@class, "pager__item--next")]/a');
-    $this->clickPreviewLinkAJAX($element, 3);
+    $this->clickPreviewLinkAJAX($next_page_link, 3);

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.

danflanagan8’s picture

Well, one more thing...

+++ b/core/modules/views_ui/tests/src/FunctionalJavascript/ViewsListingTest.php
@@ -97,10 +97,10 @@ public function testFilterViewsListing() {
-    $view_description = $enabled_view->find('css', '.views-ui-view-name h3')->getText();
+    $view_description = $enabled_view->find('css', '.views-ui-view-name strong')->getText();

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?

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mglaman’s picture

Or is that still out of scope?

I'd consider it out of scope, since we're focused on just replacing CSS selectors and XPath selectors.

The first "Navigate to next page" comment is not accurate. It looks like maybe it was accidentally copied from below?

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:

  1. +++ b/core/modules/views_ui/tests/src/FunctionalJavascript/ViewsListingTest.php
    @@ -97,10 +97,10 @@ public function testFilterViewsListing() {
    -    $view_description = $enabled_view->find('css', '.views-ui-view-name h3')->getText();
    +    $view_description = $enabled_view->find('css', '.views-ui-view-name strong')->getText();
    

    Do we need strong? Isn't .views-ui-view-name enough?

  2. +++ b/core/modules/views_ui/tests/src/FunctionalJavascript/ViewsListingTest.php
    @@ -118,15 +118,15 @@ public function testFilterViewsListing() {
    +    $this->assertEquals($view_description, $this->getSession()->evaluateScript("jQuery(document.activeElement).parents('tr').find('.views-ui-view-name strong').text()"));
    

    Ditto – text should return fine without specifying strong

mglaman’s picture

Status: Needs work » Needs review

Forgot to change the status.

danflanagan8’s picture

I insist that

The first "Navigate to next page" comment is not accurate.

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. The strong I think is trying to imitate the specificity provided by the h3, though it's likely that neither the h3 nor the strong is needed.

mglaman’s picture

Status: Needs review » Needs work
+++ b/core/modules/views_ui/tests/src/FunctionalJavascript/PreviewTest.php
@@ -197,14 +197,15 @@ public function testPreviewWithPagersUI() {
+    // Navigate to next page.
+    $next_page_link = $elements[1]->find('css', 'a');
+    $this->assertNotEmpty($next_page_link, 'Link to next page found.');
+    $this->assertEquals('Go to next page', $next_page_link->getAttribute('title'));

Oops, 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.

danflanagan8’s picture

Issue tags: +Needs reroll

Patch no longer applies to 9.4.x

yogeshmpawar’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
13.69 KB

Reroll against 9.4.x branch & addressed #31

danflanagan8’s picture

@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.

yogeshmpawar’s picture

Thanks @danflanagan8.
As per given reference given in #34 I have added an interdiff.

danflanagan8’s picture

Status: Needs review » Reviewed & tested by the community

I'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:

$result = $this->xpath('//details[@id="edit-options-more"]');
$this->assertEmpty($result, "Container 'more' is empty and should not be displayed.");

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.

mglaman’s picture

+1 to RTBC

danflanagan8’s picture

Priority: Normal » Major
Issue tags: +Drupal 10

Updating priority to Major just like @xjm did for #3248295: Taxonomy tests should not rely on Classy and adding D10 tag.

alexpott’s picture

Version: 9.4.x-dev » 9.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 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.

  • alexpott committed 38ad49d on 10.0.x
    Issue #3112547 by andypost, dww, yogeshmpawar, Lendude, bnjmnm,...

  • alexpott committed 63b32ac on 9.4.x
    Issue #3112547 by andypost, dww, yogeshmpawar, Lendude, bnjmnm,...

  • alexpott committed 529d4a6 on 9.3.x
    Issue #3112547 by andypost, dww, yogeshmpawar, Lendude, bnjmnm,...

Status: Fixed » Closed (fixed)

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