Problem/Motivation

In the past, if we wanted to test the behavior of a hook function, then we needed to implement that hook inside of a "test module." Test modules are typically located in core/modules/*/tests/modules/ directories.

#3502432: Make hook testing with kernel tests very simple provided an alternative to creating test modules for Kernel tests. Now hooks may be created as functions directly in Kernel test classes. This reduces the amount of boilerplate necessary for testing hooks, couples the hook function tightly with its related test, and gives us an opportunity to remove some test modules from Core.

The DelayCacheTagsInvalidationHooks class (core/modules/system/tests/modules/delay_cache_tags_invalidation/src/Hook/DelayCacheTagsInvalidationHooks.php) has two hook functions that appear to only be used by a Kernel test. Move the hook functions into their related test.

See the following for helpful information on making this change:

Proposed resolution

  • Search for the test class or classes that use the delay_cache_tags_invalidation module using any method you prefer, for example grep or an IDE's search.
  • Verify that the hooks are only used by one or more Kernel tests. Comment if you believe this is not true.
  • Copy the hook functions from the test module into the Kernel test class(es).
  • Delete the hook functions. If the Hook class is empty, delete the entire class. If the test module has no other code, delete the entire module.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3581550

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

dcam created an issue. See original summary.

sapnil_biswas made their first commit to this issue’s fork.

sapnil_biswas opened merge request !15255

sapnil_biswas’s picture

sapnil_biswas commented
Status: Active » Needs review

Have moved the hooks to kernel please review

dcam’s picture

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

Thank you for your contribution to Drupal!

In the future, please consider applying changes to feature branches instead of the main branch. In order to review your MR properly I have to do some branch juggling, deleting the existing main branch in my local environment and then restoring it later, because of this. Whereas if the changes were on a feature branch, then I could do a simple checkout.

The hooks were copied accurately from the hook class to the test class. Their new locations are adjacent to the test they are used by.

The test continues to pass on GitLab and in my local environment.

I grepped Core for the string "delay_cache_tags_invalidation". The only remaining uses are in the state set/get calls in the test.

The delay_cache_tags_invalidation module was empty and was deleted.

I have no feedback suggestions. This looks good to me.

sapnil_biswas’s picture

sapnil_biswas commented

thanks @dcam for reviewing, I am new to contributing here , will sure keep that in mind from the next time

dcam’s picture

dcam commented
Status: Reviewed & tested by the community » Needs work

thanks @dcam for reviewing, I am new to contributing here , will sure keep that in mind from the next time

Thank you for considering my feedback.

A separate discussion that I had made me realize this needs a little extra work. In some situations it may be unobvious which hooks belong to which tests and vice-versa. Or it may be difficult for some people to tell why those hooks are in the class at all. And if a test is removed (which happens from time to time), then we may end up with a dead code hook that serves no purpose anymore. Doing a little bit of documentation work should go a long way to fix these problems. Even in a small class this could be important because you never know when it might be expanded later. I would like for @see annotations to be added to the test docblocks for each hook that affects it. Then corresponding annotations should be made on the hook docblocks back to each test function that uses it.

I'm setting the status to Needs Work for this. @sapnil_biswas since it seems like you're monitoring the status of this issue I'll only change the status of this one. I'm not going to bother setting the other hook-move issues you worked on to Needs Work, but it would be great if you could do the same in all of them.

sapnil_biswas’s picture

sapnil_biswas commented
Status: Needs work » Needs review

Please check I’ve addressed the documentation concern by adding @see cross-references between the moved hook methods. I will be doing the same for the other issues as well, marking this for review

smustgrave’s picture

smustgrave commented

Do want to mention also @sapnil_biswas it seems you picked up all these issues that were addressing the same thing that @dcam opened up. While there is no hard rule against it since these were novice issues it would be good next time to maybe only pick up 1-2 and expand the type of tickets you work on to learn other things vs doing the same task on repeat. And it also leaves some of the novice tasks for other new users such as yourself. No problem as what's done is done but just some advice for next time. Thank you

sapnil_biswas’s picture

sapnil_biswas commented

@smustgrave thank you sir for your advice will surely follow from the next time

sapnil_biswas’s picture

sapnil_biswas commented

Have updated all the other issues as well!

dcam’s picture

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

The new @see annotations look great. I'm marking this as RTBC again.

catch’s picture

catch
he/him
Primary language English
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to main and 11.x, thanks!

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.

catch closed merge request !15255

  • catch committed d17be59d on 11.x 
    task: #3581550 Move hooks from delay_cache_tags_invalidation into the...

  • catch committed a0df2fe3 on main 
    task: #3581550 Move hooks from delay_cache_tags_invalidation into the...