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_2-9.txt1.42 KBdanflanagan8
#9 3247694-9.patch10.1 KBdanflanagan8
#2 3247694-2.patch10.15 KBdanflanagan8

Comments

danflanagan8 created an issue. See original summary.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Status: Active » Needs review
StatusFileSize
new 3247694-2.patch10.15 KB

Here's a patch that updates 6 of the 7 test classes in user.module that directly refer to classy.

The seventh reference to classy, however, is in a class (UserTranslationUITest) that extendsDrupal\Tests\content_translation\Functional\ContentTranslationUITestBase. Simply changing the default theme in the user test results in failures since the base class is still expecting classy's markup.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Assigned: danflanagan8 » Unassigned

unassigning...

danflanagan8’s picture

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

I just found something I missed in this patch. In UserAccountLinksTest there's a NEGATIVE assertion relying on classy:

$this->assertSession()->elementNotExists('xpath', '//ul[@class="menu"]/li/a[contains(@href, "user") and text()="My account"]');

There are also some negative assertions in UserAdminTest that I need to fix. for example:

$this->assertSession()->elementNotExists('xpath', static::getLinkSelectorForUser($user_a));

because the that staic function returns this:

return '//td[contains(@class, "views-field-name")]/a[text()="' . $user->getAccountName() . '"]';

That xpath needs classy.

Apparently, I had checked out the wrong git branch. I fixed the first thing above already in the first patch. And this static function is fine. There are positive assertions that use it too.

I searched the user tests for negative assertions and didn't actually find anything that I missed that would be affected by moving from classy to stark. I searched for:

  1. assertEmpty
  2. assertCount(0
  3. NotExists

And didn't see anything that looked like a negative assertion that would break when moving to stark.

I also search for xpath and css and I don't see anything left that looks like it needs classy. So I'm feeling good about this patch again. Setting back to NR.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Status: Needs work » Needs review
Related issues: +#3247901: ContentTranslationUITestBase should not rely on Classy

I made a new issue for translation UI tests here: #3247901: ContentTranslationUITestBase should not rely on Classy

I think it makes sense to handle UserTranslationUITest there.

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

mglaman
Primary language English
Location WI, USA
commented

I have just one concern about the usage of RDF data in the CSS selectors. It might not be a big deal, but it does put a dependency on RDF module manipulations versus what's provided "out of the box" by the module.

+++ b/core/modules/user/tests/src/Functional/UserPictureTest.php
@@ -121,7 +121,7 @@ public function testPictureOnNodeComment() {
-    $elements = $this->cssSelect('.node__meta .field--name-user-picture img[alt="' . $alt_text . '"][src="' . $image_url . '"]');
+    $elements = $this->cssSelect('article[typeof="schema:Article"] footer div img[alt="' . $alt_text . '"][src="' . $image_url . '"]');

@@ -135,7 +135,7 @@ public function testPictureOnNodeComment() {
-    $elements = $this->cssSelect('.comment__meta .field--name-user-picture img[alt="' . $alt_text . '"][src="' . $image_url . '"]');
+    $elements = $this->cssSelect('article[typeof="schema:Comment"] footer div img[alt="' . $alt_text . '"][src="' . $image_url . '"]');

This requires the RDF module, correct? I know the standard install includes RDF, so it's usable. My concern is just depending on RDF output in the User module.

danflanagan8’s picture

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

Good call, @mglaman. Definitely don't want to depend on rdf.

There's serious talk about removing rdf from core: #2152459: [Policy] Deprecate RDF module and move it to contrib

I'm going to set back to NW to find a better way to select.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Status: Needs work » Needs review
StatusFileSize
new 3247694-9.patch10.1 KB
new interdiff_2-9.txt1.42 KB

Here's a new patch with better selectors that don't depend on rdf.

Status: Needs review » Needs work

The last submitted patch, 9: 3247694-9.patch, failed testing. View results

danflanagan8’s picture

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

I'm setting this back to NR. Patch in #9 is green.

mglaman’s picture

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

Change to drop RDF selectors looks great, thanks @danflanagan8. Going to mark it ready.

  • lauriii committed d471b0c on 10.0.x 
    Issue #3247694 by danflanagan8, mglaman: User tests should not rely on...

  • lauriii committed 4a0768b on 9.4.x 
    Issue #3247694 by danflanagan8, mglaman: User tests should not rely on...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Fixed

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