Problem/Motivation

In more recent iterations and code changes (see related issues), new Functional test classes were added to test the three modules admin settings forms:

  • \Drupal\Tests\admin_toolbar_search\Functional\AdminToolbarSearchSettingsFormTest
  • \Drupal\Tests\admin_toolbar_tools\Functional\AdminToolbarToolsSettingsFormTest
  • \Drupal\Tests\admin_toolbar\Functional\AdminToolbarSettingsFormTest

It now appears certain Functional test classes that existed previously could potentially be merged or included in recently added test classes listed above.

This would reduce the number of Functional tests files in the project, in particular for smaller test classes, where few assertions are processed, see:

  • \Drupal\Tests\admin_toolbar_search\Functional\AdminToolbarSearchSettingTest
  • \Drupal\Tests\admin_toolbar_tools\Functional\AdminToolbarToolsAlterTest
  • \Drupal\Tests\admin_toolbar\Functional\AdminToolbarAlterTest

The setup of the tests could be grouped and the assertions from these tests could be moved to the corresponding module's settings form Functional test class.

I would "guess" these classes were added at very early stages of the project (back in 2015) as a basis for implementing Functional tests, with the intention of expanding and building on them as the module would evolve.

But at this point, we have already developed our own set of tests classes and therefore these older ones should not be needed anymore.

Steps to reproduce

Run PHPUNIT Functional tests.

Proposed resolution

1 - Merge class \Drupal\Tests\admin_toolbar_search\Functional\AdminToolbarSearchSettingTest into \Drupal\Tests\admin_toolbar_search\Functional\AdminToolbarSearchSettingsFormTest:
The values of the Admin Toolbar Search configuration (admin_toolbar_search.settings) are already updated/changed in class AdminToolbarSearchSettingsFormTest, so we would just need to check for the display of the relevant HTML IDs of the module.
Additionally, we could improve the Functional test class by testing the permission 'use admin toolbar search', instead of testing this in FunctionalJavascript \Drupal\Tests\admin_toolbar_search\FunctionalJavascript\AdminToolbarSearchTest
==> To be removed in a different ticket.
Also add checks for the route '/admin/admin-toolbar-search' to test access control with the permission.

2 - Merge class \Drupal\Tests\admin_toolbar_tools\Functional\AdminToolbarToolsAlterTest into \Drupal\Tests\admin_toolbar_tools\Functional\AdminToolbarToolsSettingsFormTest:
Move a single assertion.

3 - Merge class \Drupal\Tests\admin_toolbar\Functional\AdminToolbarAlterTest into \Drupal\Tests\admin_toolbar\Functional\AdminToolbarSettingsFormTest:
Move a single assertion.

Issue fork admin_toolbar-3540080

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

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 !168 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 clean-up some old code and an unused CSS file.
Improved a bit the Admin Toolbar Search Functional tests coverage, with a bit more documentation.

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

Hi @dydave, thanks for updating the tests! It is often one of those tasks that can linger for a long time, so getting them in tip top shape is great :)

I applied the patch, and verified that the assertions in the functions below are valid and succeed:

  • testAdminToolbarSearchSettingsForm
  • testAdminToolbarToolsSettingsForm
  • testAdminToolbarToolsToolbarController

I then changed the asserted result string, and they failed as expected. I also stepped through the HTML output pages in /simpletest/browser_output/Drupal_Tests_admin_toolbar_tools_Functional_AdminToolbarToolsToolbarControllerTest-110-[...] to verify the output.

Everything seems to work as expected (though I can't comment on the methods ...) and looks RTBC.

ressa’s picture

ressa
he/him
commented
Issue summary: View changes

Format classes as code, for better readability.

  • dydave committed 66f87070 on 3.x 
    Issue #3540080 by dydave: Automated tests: Combined several Functional...
dydave’s picture

dydave commented
Status: Reviewed & tested by the community » Fixed

Super nice @ressa! 🙏

Thanks a lot for the thorough review, it's greatly appreciated!

Following your confirmation, I went ahead and merged the merge request above at #6.

Let's keep moving, with the rest of the tickets for the tests... I've still got a few more big ones coming up. 👍
There is still quite a lot of work left. 😅

Then we'll finally be able to start makin changes and taking some more refactoring tickets, in particular for admin_toolbar_search. 😅

I'm going to get back to you on other tickets very shortly and add a few more ones related with Tests, for which I would also need your help👌
Thanks again very much for the prompt reply and 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

Nice @dydave, thanks! Great to see the test coverage is steadily improving and expanding, and that we are closer to be able to start refactoring of the admin_toolbar_search module 🙂

As always, thanks for the positive energy and great feedback, I very much appreciate it!

Status: Fixed » Closed (fixed)

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