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

CommentFileSizeAuthor
#9 interdiff-4-9.txt1.01 KBdanflanagan8
#9 3267653-9.patch15.48 KBdanflanagan8
#4 3267653-4.patch15.62 KBdanflanagan8
#3 3267653-3.patch15.62 KBdanflanagan8

Comments

danflanagan8 created an issue. See original summary.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented

I'm running into an interesting situation with Drupal\Tests\comment\Functional\CommentCSSTest which is set to use Classy. In fact, this class really is focussed on testing Classy itself. So those assertions should be moved somewhere presumably. I forget (or maybe never knew) the plan for handling situations like this. Perhaps this is a situation where we need to change to the Starterkit theme instead of Stark? I guess that's probably it.

I've also noticed that the by-viewer class is completely untested in core. Maybe that could be done too.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Status: Active » Needs review
StatusFileSize
new 3267653-3.patch15.62 KB

Here's a patch!

Regarding my comments in #2 which were really hastily scribbled...

> Perhaps this is a situation where we need to change to the Starterkit theme instead of Stark?

I left a @todo for that.

> I've also noticed that the by-viewer class is completely untested in core. Maybe that could be done too.

Way out of scope obviously so I didn't take any action on this.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
StatusFileSize
new 3267653-4.patch15.62 KB

I think I had a cs violation on that one anyway. Updated patch here.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented 
  1. +++ b/core/modules/comment/tests/src/Functional/CommentCSSTest.php
@@ -18,7 +18,15 @@ class CommentCSSTest extends CommentTestBase {
   /**
-   * {@inheritdoc}
+   * The theme to install as the default for testing.
+   *
+   * @var string
+   *
+   * @todo Change default theme to starterkit once it is stable.
+   *   The test has assertions for multiple classes that are not added directly
+   *   by the comment module. It is testing themes as much as it is testing the
+   *   comment module. The test thus cannot use stark as the default theme.
+   *   https://www.drupal.org/project/drupal/issues/3050378
    */
   protected $defaultTheme = 'classy';

    Do we have this messaging formalized anywhere?

    I don't know how often it'll be needed, but seems like something to have on https://www.drupal.org/project/drupal/issues/3083275 and also track occurrences it was used. 

  2. +++ b/core/modules/comment/tests/src/Functional/CommentThreadingTest.php
@@ -137,16 +137,22 @@ public function testCommentThreading() {
+
+    // A parent link is always accompanied by the text "In reply to".
+    // If we don't assert this text here, then the assertNoParentLink()
+    // method is not effective.
+    $pattern = "//article[@id='comment-$cid']";
+    $this->assertSession()->elementTextContains('xpath', $pattern, 'In reply to');

    This looks like an addition to the test which didn't exist before.

    While +1 on fixing the test, I'm not sure if this is in scope of the issue (drop using Classy.)

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented

Thanks, @mglaman!

This looks like an addition to the test which didn't exist before.

It is indeed an addition, but I believe it is necessary in order to allow the change in CommentThreadingTest::assertNoParentLink()

+++ b/core/modules/comment/tests/src/Functional/CommentThreadingTest.php
@@ -158,16 +164,9 @@ protected function assertParentLink(int $cid, int $pid): void {
   protected function assertNoParentLink(int $cid): void {
-    // This pattern matches a markup structure like:
-    // @code
-    // <a id="comment-2"></a>
-    // <article>
-    //   <p class="parent"></p>
-    // </article>
-    // @endcode
-
-    $pattern = "//article[@id='comment-$cid']//p[contains(@class, 'parent')]";
-    $this->assertSession()->elementNotExists('xpath', $pattern);
+    $pattern = "//article[@id='comment-$cid']";
+    // A parent link is always accompanied by the text "In reply to".
+    $this->assertSession()->elementTextNotContains('xpath', $pattern, 'In reply to');
   }

Without classy there's no parent class and the "In reply to" text was the only reasonable thing I could see to key off of for testing the non-existence of a parent. But asserting that this text is NOT there is only useful if we also assert the text IS there in the "sister" function CommentThreadingTest::assertParentLink(). That's the code I added that looks out of scope. But I think we need it.

These two functions (assertParentLink/assertNoParentLink) are called out in this old issue for being really weird: #1847540: [META] Clean up comment module tests and decouple from node

Do we have this messaging formalized anywhere?

No, and I like your idea of getting something formalized. I'll post over there.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented

Thanks @danflanagan8. +1 from me then. I don't think it's RTBC ready until we resolve wording for the "Needs CSS classes in tests, use starterkit" @todo

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Status: Needs review » Needs work

Cool! I'm going to set it back to NW since we still need to update the @todo whenever we settle on language.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Status: Needs work » Needs review
StatusFileSize
new 3267653-9.patch15.48 KB
new interdiff-4-9.txt1.01 KB

Here's a patch with the updated @todo.

It follows the pattern proposed by @xjm in the parent issue: #3083275-25: [meta] Update tests that rely on Classy to not rely on it anymore

I created a followup that is referenced in the updated @todo: #3267890: Update CommentCSSTest to use starterkit

Ready for review again!

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Status: Needs review » Reviewed & tested by the community

Going to RTBC. We've got the standardized @todo messaging when we have to remain on classy and the follow up created.

  • lauriii committed 089cd0b on 10.0.x 
    Issue #3267653 by danflanagan8, mglaman: Comment tests should not rely...

  • lauriii committed cfd82a0 on 9.4.x 
    Issue #3267653 by danflanagan8, mglaman: Comment tests should not rely...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Fixed 
+++ b/core/modules/comment/tests/src/Functional/CommentCSSTest.php
@@ -18,7 +18,14 @@ class CommentCSSTest extends CommentTestBase {
+   * @todo This test's reliance on classes makes Stark a bad fit as a base
+   *   theme. Change the default theme to Starterkit once it is stable.

+1 for skipping this one.

Committed 089cd0b and pushed to 10.0.x. Also cherry-picked to 9.4.x. Thanks!

Status: Fixed » Closed (fixed)

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