Fix Drupal.Commenting.FunctionComment.Missing in module tests

Linting passed, so setting for review.

Reviewed the MR. Changes look good to me. Also ignoring the sniff in files using testN() seems to be fine.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Time to get some reviews here.

Re-ran test failures and they were all random.

Was previously RTBC restoring status

Found a couple of nits.

@longwave, thanks for the review. I made the corrections and checked if any other changes were needed due to the recent update of Coder.

Feedback appears to be addressed

Midway through my review. The hook order tests need so much work that IMO we might want to pull them out into their own issue, especially since this is one of those test fixture sets where understanding exactly what is under test is important.

OK, done reviewing now. Phew, this one is massive! Given the 290 total LOC diffstat of all new docs that furthermore requires you to read not only the context lines of the method but also entirely separate code of the tests these fixtures are for, I think the scope of this is too big. There are two particularly problematic tests that I think should be moved to their own separate issues. Thanks!

It's not module tests; it's test modules.

I'm going to move the changes for HookOrder to another issue. Assigning to myself for that and to review other local changes before pushing.

Removed the hook related ignores, except one for what is an unused function, \template_preprocess_test.

How are these for comments on each?

diff --git a/core/modules/system/tests/modules/module_test_procedural_preprocess/module_test_procedural_preprocess.module b/core/modules/system/tests/modules/module_test_procedural_preprocess/module_test_procedural_preprocess.module
index 1e7665b412d..99f405dbf76 100644
--- a/core/modules/system/tests/modules/module_test_procedural_preprocess/module_test_procedural_preprocess.module
+++ b/core/modules/system/tests/modules/module_test_procedural_preprocess/module_test_procedural_preprocess.module
@@ -7,14 +7,25 @@
 
 declare(strict_types=1);
 
+/**
+ * Confirms that template_preprocess_HOOK functions are gathered and executed
+ * when there is no initial preprocess key set on hook_theme.
+ */
 function template_preprocess_test($arg): mixed {
   return $arg;
 }
 
+/**
+ * Confirms that procedural hook_preprocess() hooks are gathered and executed.
+ */
 function module_test_procedural_preprocess_preprocess($arg): mixed {
   return $arg;
 }
 
+/**
+ * Confirms that procedural hook_preprocess_HOOK() hooks are gathered and
+ * executed.
+ */
 function module_test_procedural_preprocess_preprocess_test($arg): mixed {
   return $arg;
 }

I'll create a follow up to ensure the template preprocess coverage is as expected.

Here is the follow up for the test coverage: 📌 Confirm there is a test that procedural preprocess functions are executed properly in modules Active

I commented an hour ago but forgot to save. Here is that comment.

@nicxvan, thanks for prompt response to our Slack conversation about this. I updated your suggestions in the MR. In particular, wanting to add more detail so that the last two functions do not have identical comments. This was pointed out by xjm and is why I have moved adding comments to the HookOrder related test modules to a new issue.

Linting passed, back to Needs review.

Please link me to the hook order documentation issue when you create it. They are pretty difficult to hold all in your head so a summary of expected orders would be great, just not sure where.

I did manually map them out and confirm them all as part of the hook ordering issue they were added in, but I couldn't figure out where to document it.

The ones I wrote I kept much more simple with only a couple hooks per test, but this was more comprehensive and realistic test coverage.

@nicxvan, the diff in #13 has the same comment for two of the functions and @xjm wants a specific message for each function. One that explains what it is testing. I can appreciate that, there is nothing in the file to explain why two very similar functions are needed.

Yep, I resolved my threads. They are different hooks but I think it's clear from the context what they are.

The test called them prefix because Registry does. But I don't think I've seen the module name called a prefix other than that. Same for HOOK being called a suffix.

I don't think that's worth holding this up, but there is one other follow up needed for the router test.

Once those are linked and is green this is rtbc as far as I am concerned.