Problem/Motivation

The module currently does not have a common method for test classes to check whether a specific link exists or not in the admin toolbar.

Each class currently has its own "copy" of the same methods/code: protected function assertMenuHasHref($url)

The same goes for assertMenuDoesNotHaveHref.

The methods are all marked as protected, so they could not be called from a separate test class, requiring another set of methods to be implemented for the test, which is nor practical or sustainable in terms of maintenance.

Proposed resolution

Add the new trait AdminToolbarHelperTestTrait to provide a common method for module's test classes to check that a specific link exists or not in the admin toolbar.

The trait could then be used by any test class whether Functional or FunctionalJavascript and provide a standard method for asserting a link exists in the admin toolbar.

Additionally, we could improve the method of asserting a link exists by adding more specific conditions, such as the link text, display position (order) or CSS classes.

Replace current implementations in \Drupal\Tests\admin_toolbar\Functional\AdminToolbarToolsSortTest by using the trait's method.

Issue fork admin_toolbar-3549663

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

dydave created an issue. See original summary.

dydave opened merge request !171

dydave’s picture

dydave commented
Status: Active » Needs review

Quick follow-up on this issue:

All the changes detailed in the issue summary have been implemented and described in the merge request MR !171 above at #2.

Since all the tests and jobs still seem to be passing 🟢, moving issue to Needs review as an attempt to get more testing feedback and reviews.

Overall, this merge request is really just a "routine" maintenance task trying to refactor and improve some old test code so it could be used by new test classes.

Feel free to let us know if you have any comments, questions or concerns on any aspects of this issue or the suggested changes in the merge request, we would surely be glad to help.
Thanks in advance!

ressa’s picture

ressa
he/him
commented
Status: Needs review » Reviewed & tested by the community

Thanks again @dydave for the continued test updates, I really appreciate it!

I applied the patch, and the functions assertAdminToolbarMenuLinkExists and assertAdminToolbarMenuLinkNotExists are used in the assertions in tests/src/Functional/AdminToolbarToolsSortTest.php where menu items are created an deleted as expected.

I verified that assertAdminToolbarMenuLinkExists and assertAdminToolbarMenuLinkNotExists work, by running them before the menu or content entity bundle are added or removed, and the test failed, as expected.

I stepped through the HTML output, and checked the menus in GUI and source as well, and the elements are present, and not, as expected.

It's great to have a global method to check links :)

As a follow up, should the remaining assertMenuHasHref be switched to use this new solution?

$ grep -rinoE '.{0,10}assertMenu.{0,10}' .
./admin_toolbar_search/tests/src/FunctionalJavascript/AdminToolbarSearchTestBase.php:186: function assertMenuHasHref($u
./admin_toolbar_search/tests/src/FunctionalJavascript/AdminToolbarSearchTestBase.php:202: function assertMenuDoesNotHav
./admin_toolbar_search/tests/src/FunctionalJavascript/AdminToolbarToolsSearchTest.php:169:   $this->assertMenuDoesNotHav
./admin_toolbar_search/tests/src/FunctionalJavascript/AdminToolbarToolsSearchTest.php:173:   $this->assertMenuHasHref('/

  • dydave committed 62c44471 on 3.x 
    Issue #3549663 by dydave: Automated tests: Added trait '...
dydave’s picture

dydave commented
Title: Tests: Add trait AdminToolbarHelperTestTrait » Automated tests: Add trait AdminToolbarHelperTestTrait
Status: Reviewed & tested by the community » Fixed

Thanks a lot @ressa!

Please keep the same testing protocol for all other tickets, it's super helpful! 🙏

After receiving your confirmation, I added a quick commit to ignore PHPSTAN errors that were unrelated to these changes 👌

 ------ ----------------------------------------------------------------------- 
  Line   admin_toolbar.module                                                   
 ------ ----------------------------------------------------------------------- 
  99     Property Drupal\Core\Menu\MenuLinkTreeElement::$options (array<string  
         >) does not accept array<array<string, list<string>>|string>.          
         🪪  assign.propertyType                                                
  100    Property Drupal\Core\Menu\MenuLinkTreeElement::$options (array<string  
         >) does not accept array<array<string, list<string>|string>|string>.   
         🪪  assign.propertyType                                                
 ------ ----------------------------------------------------------------------- 
 [ERROR] Found 2 errors

Not sure why these errors are popping up now 😅.... Probably because there were some recent updates with the PHP or PHPSTAN versions or standards with the newer supported Core versions tested on Gitlab CI.
But since we were already ignoring the offsetAssign.dimType error above in the code, we might as well just ignore these assign.propertyType ones for now.
Note that this method introduces other errors for previous major core version (10.5.x):

 ------ ----------------------------------------------------------------------- 
  Line   admin_toolbar.module                                                   
 ------ ----------------------------------------------------------------------- 
  100    No error with identifier assign.propertyType is reported on line 100.  
  102    No error with identifier assign.propertyType is reported on line 102.  
 ------ ----------------------------------------------------------------------- 
 [ERROR] Found 2 errors

So we're probably going to have to look into this a bit closer in a separate ticket 😖
Try to find a better way to conditionally support different PHPSTAN versions 😅

As a follow up, should the remaining assertMenuHasHref be switched to use this new solution?

Thanks a lot @ressa for being very thorough and taking the time to carefully review the code changes! 🙏

All the affected files are in the admin_toolbar_search module's tests and they are going to be completely revamped in the next tickets and merge requests.
So let's keep them as is for now 👌
 

For now, I went ahead and merged the MR above at #5.
Let's keep moving with the module's tests coverage, then more refactoring and tickets in the issue queue. 👍

More Automated Tests tickets and MRs coming up! 🥳
Thanks again very much for your great help @ressa! 🙏

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.

ressa’s picture

ressa
he/him
commented

You're welcome @dydave, I am glad I could help! I'll do my best to keep the same testing protocol in future tickets.

It sounds complicated with PHPSTAN ... the automated tools work great, until the complexity gets too much, it seems 😅 But great that you found a solution, and thanks for diligently documenting the choices and problems in this issue in text and feedback from the tools -- it might come in handy later, since there is now some background.

And a good plan to take care of assertMenuHasHref in the admin_toolbar_search module's tests in a future revamp of the tests, that makes a lot of sense. Thanks @dydave for your always positive approach and thorough feedback, it boosts the motivation to contribute more to Drupal a lot.

Status: Fixed » Closed (fixed)

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