AssertMenuActiveTrailTrait should not rely on classy/bartik

Hi, I'm starting working on this issue at Drupalcon Portland

If we dig down AssertMenuActiveTrailTrait.php has only one function that is only used inside AssertBreadcrumbTrait::assertBreadcrumb. AssertBreadcrumbTrait.php is used by 9 different tests:

block_content/tests/src/Functional/BlockContentTypeTest.php
dblog/tests/src/Functional/DbLogTest.php
help_topics/tests/src/Functional/HelpTopicTest.php
node/tests/src/Functional/NodeTitleTest.php
node/tests/src/Functional/NodeTypeTest.php
system/tests/src/Functional/Menu/BreadcrumbFrontCacheContextsTest.php
system/tests/src/Functional/Menu/BreadcrumbTest.php
taxonomy/tests/src/Functional/TermTest.php
taxonomy/tests/src/Functional/TermTranslationTest.php

7 of them already use 'stark' as the default theme, 1 hasn't a default theme (BreadcrumbTest.php) and only 1 uses 'classy' (BreadcrumbFrontCacheContextsTest.php). Moreover, BreadcrumbFrontCacheContextsTest.php actually does not use any of the functions from AssertBreadcrumbTrait.php (I confirmed this by removing the use statement and running the test locally).

So, with that I believe that AssertMenuActiveTrailTrait.php may not need to be refactored to work with Stark.

Hi, I am also working on this.

Working on this at Portland2022

Also found that menu-item--active-trail is still in Olivero, so we questioning end goal of ticket, because we can't just change it to use menu__item--active-trail because it would break the Olivero test as well.

Wondering next step?

FYI. This just got committed a little bit ago. #3276618: Olivero sets inconsistent classes for active trail between menu and book. And changes the class in Olivero. As well as provides a temporary work around for the test trait. This issue is still necessary to make the tests not hard-coded to any particular theme, but is no longer blocking Olivero.

In the closely related issue #3248309: AssertBreadcrumbTrait should not rely on Classy, here's a comment I wrote at the time:

I should not[e] that this trait uses another trait called AssertMenuActiveTrailTrait. That trait has one function, assertMenuActiveTrail(), which will never work with stark because it tests for the menu-item--active-trail class, which stark only puts on the toolbar.

This turns out not to be a big deal though. This function is only called during one test class, which is BreadcrumbTest. (That's the only test that passes a $tree to AssertBreadcrumbTrait::assertBreadcrumb(). That test uses the standard profile and it can continue to use the standard profile. So we can just leave it as is.

The trait is used to test Classy/Bartik, so changing it to work with Stark doesn't make sense to me. It should either (1) be changed to work with Olivero or (2) be modified to allow additional parameters.

For (2) we would add optional parameters to AssertBreadcrumbTrait::assertBreadcrumb and AssertMenuActiveTrailTrait::assertMenuActiveTrail that would be used to specify the active trail class and the active class. They would default to menu-item--active-trail and is-active respectively.

Updating status to active since there is nothing to review.

Adding tag

I'm gonna work on @danflanagan8 suggestion at #14

As suggested on #14 I added the optional parameters active_trail_class (w/ default value 'menu-item--active-trail') and active_class (w/ default vaule 'is-active') to AssertBreadcrumbTrait::assertBreadcrumb and AssertMenuActiveTrailTrait::assertMenuActiveTrail making the needed substitutions.

I forgot to change status.

@angregp, the change looked good to me but something is off. The patch failed to apply. Back to NW.

Okay, I might be some commits behind. I'll update my repo and reroll it :)

Updated patch!

Sorry, forgot one more update.

We'll see, but I think that patch is going to fail, that is if I am correctly understanding what was happening in #3276618: Olivero sets inconsistent classes for active trail between menu and book.

I would expect you have to change the assertion around line 292 of Drupal\Tests\system\Functional\Menu::BreadcrumbTest to use the new arguments. Something like

$this->assertBreadcrumb($link_path, $trail, $term->getName(), $tree, 'menu__item--active-trail');

We can wait and see or you could run Drupal\Tests\system\Functional\Menu::BreadcrumbTest and see if it fails.

Hurray! It's good to be wrong. This patch is RTBC.

I was under the impression that Drupal\Tests\system\Functional\Menu::BreadcrumbTest was already using Olivero. Not true. Still using Bartik. So that change I mentioned in #23 will have to be made whenever Olivero is added to the standard profile.

Updated the code in \Drupal\Tests\system\Functional\Menu\BreadcrumbTest::testBreadCrumbs now that Olivero is the default theme in the standard profile.

That sure looks like a random fail, or if not random certainly unrelated. Update looks good to me.

Agree that this is at least not related to the changes in this issue. See #3278215: (Not so) Random test failures SettingsTrayIntegrationTest

Radom fail. Requeueing tests and restoring status.

Unrelated random fail again, restoring status.
(You may notice that the third test fail was even from a different test.)

Committed and pushed 3eedc9628e to 10.0.x and 537c279861 to 9.5.x and 9fcb0a0221 to 9.4.x. Thanks!