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 CkeditorTestHooks class (core/modules/ckeditor5/tests/modules/ckeditor_test/src/Hook/CkeditorTestHooks.php) has a hook function that appears to only be used by a Kernel test. Move the hook function into its related test.

See the following for helpful information on making this change:

Proposed resolution

  • Search for the test class or classes that use the ckeditor_test module using any method you prefer, for example grep or an IDE's search.
  • Verify that the hook is only used by one or more Kernel tests. Comment if you believe this is not true.
  • Copy the hook function from the test module into the Kernel test class(es).
  • Delete the hook function. 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-3581541

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

sapnil_biswas’s picture

sapnil_biswas commented
Status: Active » Needs review

moved the hooks to kernel, marking this for review please let me know if there is requirement of any further changes

dcam’s picture

dcam commented
Status: Needs review » Needs work

I've left a couple of comments on the merge request. They have suggestions for reverting some changes that were made in your PHPCS commit that are unrelated to the work of this issue.

If you aren't aware, we try to avoid unrelated changes in merge requests. By doing this we help prevent unnecessary rebases for ourselves and others. It also reduces the cognitive load for reviewing so no one has to wonder "Why was this change made and is it important?" This issue is a good example. Instead of very quickly reviewing the MR last night I had to go track down the commit where the changes were made, then verify that they aren't necessary.

There are days when I might let one or two minor things like this slide, but I would rather use this as an opportunity to educate. Also, you should check the options you're using for running PHPCS. I tried it on my local environment and it did not attempt to alter those lines. It's probably as simple as copying core/phpcs.xml.dist to your project root.

I'm setting the status to Needs Work for the noted changes to be reverted.

sapnil_biswas’s picture

sapnil_biswas commented
Status: Needs work » Needs review

@dcam Thank you sir for your valuable advice this will really help me a ton in my upcoming contributions

dcam’s picture

dcam commented

Thank you for considering my feedback.

The hook was copied accurately from the former class to the test.

The test passes on GitLab and on my local environment.

The ckeditor_test module has been deleted.

I grepped Core for the string "ckeditor_test". There are no remaining instances.

The new @see annotations reference the correct functions.

It looks good to me.

dcam’s picture

dcam commented
Status: Needs review » Reviewed & tested by the community
catch’s picture

catch
he/him
Primary language English
commented
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 !15250

  • catch committed e993f99c on 11.x 
    task: #3581541 Move hooks from ckeditor_test into the Kernel test

By:...

  • catch committed 8c9e1045 on main 
    task: #3581541 Move hooks from ckeditor_test into the Kernel test

By:...

Status: Fixed » Closed (fixed)

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