Problem/Motivation

Currently, very little logic of the Admin Toolbar Tools' class \Drupal\admin_toolbar_tools\Plugin\Derivative\ExtraLinks is being covered by automated tests.

The file is more than 800 lines long 😅, with one very large method:
\Drupal\admin_toolbar_tools\Plugin\Derivative\ExtraLinks::getDerivativeDefinitions()
containing a lot of cases and pieces of logic allowing to add extra links to the admin menu, such as all the extra links under 'Structure' for content entities, Edit, Manage display, Manage fields, etc...

It seems the function has grown with the superposition of layers of code, with duplications or redundancy and could use some refactoring at some point, see for example:

The 'Add {entity type}', 'All types' and other types of links could perhaps be grouped and/or refactored in more generic cases, see for example one of the issues above.

Mostly the function could be summed up to the following types of extra links:

  • Content entities links, under 'Structure' and 'Content', such as for Block content, Media, Menu, Node, etc...
  • User related links, under 'People', such as roles/permissions links, etc...
  • Custom config menu links, under various menus, such as themes settings, config import/export, language detection, etc...

 
Some cases related with content entity links are currently tested in the following classes:

\Drupal\Tests\admin_toolbar\Functional\AdminToolbarToolsSortTest:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.x/tests/src/Fu...
Testing the display, creation and deletion of a Media type and Menu.
Testing the display order of created menus in the admin menu ==> Ordered by label.

\Drupal\Tests\admin_toolbar_search\FunctionalJavascript\AdminToolbarToolsSearchTest:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.x/admin_toolba...
Testing some of module's features through its integration with Admin Toolbar Search: 'SearchLinks' class.
Display, creation and deletion of a Media type, Menu and Node type.

Testing specific entity types is really limited in terms of coverage, since the method getDerivativeDefinitions has a lot of module specific code, such as:
if ($this->moduleHandler->moduleExists('node')) for node types
if ($this->moduleHandler->moduleExists('taxonomy')) for taxonomy vocabularies
if ($this->moduleHandler->moduleExists('menu_ui')) for menus
if ($this->moduleHandler->moduleExists('block_content')) for block content types
and so many more....

For example, there are specific cases for:

None of which are currently tested...
and everything is hard coded, with code duplication a bit everywhere 😅

Steps to reproduce

Run PHPUNIT Tests for the admin_toolbar_tools module.
Run PHPUNIT Tests for the admin_toolbar_search module.

Proposed resolution

We should consider a more generic method to try executing as much of the classes' code as possible, enter in as many 'if' cases as possible and test all supported content entities.

Additionally, since the 'ExtraLinks' and 'SearchLinks' classes are very close, we should probably try covering them in the same test classes.

The 'SearchLinks' class should actually belong to the admin_toolbar_tools module and there should be a more generic way to add extra links to the admin_toolbar_search, but this should probably be addressed in a different issue and is not the object of this ticket.

What should be tested?

  • Test all the links added under "Structure", for all supported and tested entity types
  • Test all the links added under "Content", for all supported and tested entity types
  • Test all the links add for user, roles and permissions
  • Test custom extra links not tested in other methods.
  • Test Admin Toolbar Search controller and service class 'SearchLinks'.
  • Test updating and deleting entity type bundles.

Test all the modules supported by classes 'ExtraLinks' and 'SearchLinks' except external integrations:
devel, devel_php, project_browser and webprofiler.

Creating custom entities for the tests (triggering 'entity_insert' hooks) should also allow testing all the code in file admin_toolbar_tools.module, such as:
function admin_toolbar_tools_entity_insert:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.x/admin_toolba...
which also calls code in class \Drupal\admin_toolbar_tools\AdminToolbarToolsHelper allowing to test this class as well completely, except for the "Show Local Tasks" feature (method 'buildLocalTasksToolbar').

Overall, except for the "Show Local Tasks" feature (to be addressed in related test ticket) and external (module) integrations, these tests should allow to almost completely cover the Admin Toolbar Tools and Admin Toolbar Search modules.
 

Issue fork admin_toolbar-3550604

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 !173

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 !173 above at #2.

Mostly: added two new Functional tests files:

  • admin_toolbar_tools/tests/src/Functional/AdminToolbarToolsExtraLinksCustomTest.php
  • admin_toolbar_tools/tests/src/Functional/AdminToolbarToolsExtraLinksTest.php

with a generic method for testing all supported entity types through a centralized test matrix containing all the necessary information for each entity type to be tested defined in added test trait:
admin_toolbar_tools/tests/src/Traits/AdminToolbarToolsEntityCreationTrait.php

Entity type bundles currently tested:

  • Block content type (block_content)
  • Comment type (comment)
  • Contact form (contact)
  • Media type (media)
  • Menu (menu)
  • Content type (node)
  • Taxonomy vocabulary (taxonomy)
  • User role (user)
  • View (views)

 
The idea behind the test matrix is to try keeping everything related to the entity types in one place, making it easier to maintain and update as needed, for example, adding or removing an entity type to be tested, or testing different 'field_ui' operations, etc...
See new trait: Drupal\Tests\admin_toolbar_tools\Traits\AdminToolbarToolsEntityCreationTrait
https://git.drupalcode.org/issue/admin_toolbar-3550604/-/blob/3550604-au...

The important thing to know is that the entity bundles are now dynamically generated with mostly two parameters controlling the tests, see in trait method 'setUp':
\Drupal\Tests\admin_toolbar_tools\Traits\AdminToolbarToolsEntityCreationTrait::setUp():
- The variable '$max_bundle_number' controls the configuration setting 'max_bundle_number' for the tests.
- The variable '$entity_bundle_ids_count' controls the number of entity type bundles to generate for the test. The greater its value and the longer the tests will take to run, since there will be more bundles to create for each entity type. Keep this value above the 'max_bundle_number' to fully test the expected behaviour of the module.
 

Mostly, the complexity of these tests stems from the heterogeneity of the code and logic in the ExtraLinks class... It's quite a mess actually 😅... with lots of exceptions and cases.

For example, the 'max_bundle_number' config variable currently only works with links added under 'admin/structure', but not with the ones added under 'admin/content' or not with config entities, such as Views or User roles... which doesn't really make sense ?!
In other words, if you limit the menu to 10 bundle items and have 15 content types, the 15 links will still be displayed under the 'Content' menu...
I had assumed this feature was initially meant to limit the number of bundle links displayed in the menu... 😅

I think we will definitely need to come back on this feature at some point and see if there could be any other potential solutions.... Or at the very least, try to fix/standardize a bit this feature.

Refactoring some code, for example #3028213: generalize creation of 'add ENTITY' and 'add BUNDLE' links, would also help having fewer exceptions in tests and thus fewer properties in the test matrix.

The 'Add {entity_type}' links would also need to be standardized: Some entity types do not currently have an add link, such as Block content or Comment types ==> Part of the refactoring work 👌

Bonus: +1000 for TDD: Test Driven Development! \o/ 🥳

Thanks to the tests, a few "bugs" could be identified and fixed along the way:

\Drupal\admin_toolbar_tools\Plugin\Derivative\ExtraLinks:
- Standardized the display order of the 'Add entity type' and 'All types' links, which was not the same for menus.
Now all entity types have the same order: first 'All types', then 'Add entity type'.
- Standardized the display order of the extra links operations for User roles: 'Edit permissions', 'Delete'. The 'Delete' link is the last of the list in general.

\Drupal\admin_toolbar_tools\AdminToolbarToolsHelper:
- Added support for rebuilding extra links for 'user_role' and 'view' types when an item is created, updated or deleted.

\Drupal\admin_toolbar_search\SearchLinks:
- Added missing 'field_ui' operation link 'Manage permissions', copied and adapted from class 'ExtraLinks'.
- Standardized entity type label for extra links added for 'menu_ui': Replaced 'Menus' with 'Menu'.

In general, all the links are tested with url, link text, position and CSS classes, wherever possible.

The test class \Drupal\Tests\admin_toolbar\Functional\AdminToolbarToolsSortTest was removed in favor of the new \Drupal\Tests\admin_toolbar_tools\Functional\AdminToolbarToolsExtraLinksTest which does the same checks:
- Update and delete entity type bundles: Ensure the extra links are rebuilt when entity bundles are updated or deleted.
- Test the display order of the links.

Some weird interactions with the contact module's 'personal' default contact form: Since it does not have an 'Edit' route, it just doesn't display in the menu, even though it has other operations links... Maybe to be investigated in a different issue 😅
Therefore, for the time being, certain weights or label prefixes were used to control the display order of the entity type bundles created for the tests, see:
https://git.drupalcode.org/issue/admin_toolbar-3550604/-/blob/3550604-au...
 

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 would allow the module to greatly increase its Functional tests coverage to almost 100% 🥳
Which would be a great step forward allowing us to process more changes safely and more particularly, work on refactoring modules admin_toolbar_tools and admin_toolbar_search.
Not to mention the great help with the maintenance in general, with core version updates, deprecated functions, etc...
This initial base of tests would definitely be a great improvement! 👍

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!

joachim’s picture

joachim commented
Status: Needs review » Needs work

I don't think it's a good idea for a tests issue to be making changes in non-test code as well. Those probably need their own issues.

> AdminToolbarToolsConstants

It looks like this is only used for tests? It makes more sense to put this trait in tests/src, or in a test base class.

Also, what are the functional tests checking? Could this be done with Kernel tests instead? We presumably already have Functional tests that test the menu works correctly. Here we just care that certain conditions produce certain menu item plugins.

dydave’s picture

dydave commented
Status: Needs work » Needs review

Thanks Joachim (@joachim) for taking the time to look at this issue, it's greatly appreciated to have some feedback.

I don't think it's a good idea for a tests issue to be making changes in non-test code as well. Those probably need their own issues.

I've pulled out of the merge request all non tests files and created the following 3 corresponding tickets on which we would greatly appreciate your reviews and feedback:

 
I have not removed the files from this MR just yet, since the tests would break as these changes are now effectively tested, which is not the case with the current tests base.

> AdminToolbarToolsConstants

It looks like this is only used for tests? It makes more sense to put this trait in tests/src, or in a test base class.

Done: Moved constants class 'AdminToolbarToolsConstants' under the 'tests' folder.

Also, what are the functional tests checking? Could this be done with Kernel tests instead? We presumably already have Functional tests that test the menu works correctly. Here we just care that certain conditions produce certain menu item plugins.

Indeed, you are right. Maybe I didn't explain correctly what these new test classes are for:

This is a refactoring of current existing tests classes with admin_toolbar_tools:
\Drupal\Tests\admin_toolbar\Functional\AdminToolbarToolsSortTest
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.6.2/tests/src/...

\Drupal\Tests\admin_toolbar_search\FunctionalJavascript\AdminToolbarToolsSearchTest
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.6.2/admin_tool...

This is more of an integration tests suite to improve the tests coverage of the integration of the Admin Toolbar Tools module with other modules:

  • admin_toolbar
  • admin_toolbar_search
  • config
  • block_content
  • comment
  • contact
  • field_ui
  • language
  • media_library
  • menu_ui
  • node
  • taxonomy
  • user
  • views_ui

 
Very little of the integration code is actually currently tested in the module.
None of the issues listed above could have been identified without having a complete integrations test, see for example issues with User roles or Views.

These new Functional tests classes are a rewrite of the existing classes with many improvements and much more complete coverage of the executed code.

Maybe we could work on writing more precise Kernel tests, but I'm not sure exactly how that would work and how much time that could take.... I haven't looked further than that...

But what I'm sure about is that what we have in the merge request right now is much more complete than the existing tests and would help greatly keeping the module well maintained or taking on more refactoring tickets.
 

I'm setting this back to Needs review even though these changes would have to be merged after the issues above have been fixed, so the MR could be rebased properly, without module's code files in the DIFF.

Feel free to let us know if you have any questions or concerns on any aspects of this comment or the suggested changes, we would surely be glad to hear your feedback.
Thanks in advance!

dydave changed the visibility of the branch 3550604-automated-tests-add-debug-d9 to hidden.

dydave’s picture

dydave commented

I can see a bit better what Joachim (@joachim) meant with using Kernel tests instead of Functional....

In particular, I've had a small sample of the difficulty of getting these tests to stay compatible with multiple core versions:

I had to add special cases for supporting tests for D9 and revert the change suggested above moving the constants to the tests folder ...
It seemed to work with D10 and D11, but not D9 😖 (with a class not found error)...
See the two last commits in the merge request.

Overall, the great complexity of these tests mostly stems from the "incomplete" or "unbalanced" implementation of the max bundle number feature...
Otherwise, these tests should be much more complete than the existing ones and should provide a much better coverage of module's integration with other core and contrib modules.

Therefore, let's move forward with what we have in this MR and we'll see how to make this base of tests evolve in the future.

Pending merge ....

  • dydave committed b7f9d010 on 3.x 
    Issue #3550604 by dydave: Automated Tests: Added Functional tests for...
dydave’s picture

dydave commented
Status: Needs review » Fixed

Quick follow-up:

Since all the tests and jobs were still passing 🟢, including the D9 pipeline, I went ahead and merged the changes above at #8 🥳

This should allow us to intercept changes to core modules or APIs, much easier, with the different core versions being tested in pipelines, so we might be able to react quicker and fix any potential compatibility or support issues with newer core versions.

We can certainly revisit these test classes in the future, to maybe move some of them to Kernel tests, but for the time being, we could probably consider this issue as Fixed, for now.

Feel free to let us know if you have any questions or concerns on any aspects of this ticket or the project in general, we would surely try answering as soon as possible.
Thanks again everyone for the great help and feedback on this issue! 🙏

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

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

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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