(see also #3369958: Improve unit test coverage for ModuleHandler)

Problem/Motivation

The hook system is currently covered directly by Drupal\Tests\Core\Extension\ModuleHandlerTest, and indirectly by various module tests that use hooks.

There are some shortcomings with this, which become apparent when we try to refactor the hook system, as in #3368812: Hux-style hooks, proof of concept.

  • The coverage is insufficient: There are some bugs we can introduce that do not cause test fails in ModuleHandlerTest.
  • As a unit test, this one covers the class itself, instead of the "hook system". Currently this is mostly the same, but if we refactor it, the "hook system" will be a combination of classes, and their wiring as services.
  • The test relies heavily on mocking, including mocking of protected methods in ModuleHandler.
  • As a consequence, refactoring the hook system would require big changes in the ModuleHandlerTest, which makes it a poor solution for verifying that existing hooks still work as expected.

In #3369958: Improve unit test coverage for ModuleHandler I propose changes to the Drupal\Tests\Core\Extension\ModuleHandlerTest.
But this does not change the main problem: As a unit test, the test covers specific classes, instead of the system in its real-world composition.

Steps to reproduce

See #3368812: Hux-style hooks, proof of concept how many changes are needed in ModuleHandlerTest, as a consequence of the refactoring.

Proposed resolution

Add a kernel test to cover the hook system.

Remaining tasks

Is there anything in #7 to do?
Do no add a new word to dictionary.txt

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3381785

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

donquixote created an issue. See original summary.

donquixote opened merge request !4606

donquixote’s picture

donquixote commented

The kernel tests are based on the unit tests in #3369958: Improve unit test coverage for ModuleHandler.
Some methods are not converted yet.

I already tested how the test will change with the Hux MR.
Most of it remains unchanged, because we don't rely on mocks, and we use the real service definitions, instead of custom composition.

donquixote’s picture

donquixote commented
Related issues: +#3379488: Add kernel tests for event system

There is one commit borrowed from #3379488: Add kernel tests for event system.

donquixote’s picture

donquixote commented

Maybe we can drop some parts of Drupal\Tests\Core\Extension\ModuleHandlerTest?
Not sure.

donquixote’s picture

donquixote commented
Related issues: +#3381933: Establish "core/tests/$type" as a canonical place for test extensions
donquixote’s picture

donquixote commented

This MR introduces other changes:

  • Possibility to declare test module names per test method, using an attribute.
  • Possibility to use "fake" modules that do not actually exist in the file system.

This works, but needs to be cleaned up a bit and made universally usable by all tests.

It would be interesting to have virtual modules in stream wrappers.
To do this we need to allow modules to have a different root path.

donquixote’s picture

donquixote commented
Status: Active » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Was hoping #3381933: Establish "core/tests/$type" as a canonical place for test extensions would land first but I'll go ahead and mark this. If this lands before guess the other could be closed.

quietone’s picture

quietone commented
Issue summary: View changes
Status: Reviewed & tested by the community » Needs work

I'm triaging RTBC issues. I read the IS, the comments and the MR (not a code review).

Always good to improve testing, thanks!

Comment #7 states "This works, but needs to be cleaned up a bit and made universally usable by all tests." It seems this should be at needs work?

I left some comments in the MR, mostly about comments. However, I see that this is adding a word to the dictionary. We should not be doing that and I have asked a question about that. Because of that, and possibly #7, I am setting this back to needs work.

nicxvan’s picture

nicxvan commented
Component: base system » extension system

Do we still need this? There are now kernel tests for hooks.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

nicxvan’s picture

nicxvan commented
Status: Needs work » Closed (outdated)

We have since gotten these tests in.

The core test scanner has its own issue linked.

This also has an attribute for installing modules, I'm not sure that is necessary.

I've applied credit.

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.