[Sep 12, 2022] Add missing docblocks for test methods ::setUp() and ::tearDown()

There are hundreds of setUp methods without docblocks, if we are to fix this I think we should add them all in one go instead of one by one.

Coding standards say that all methods should have docblocks, but Drupal core has never fully implemented this and the Drupal.Commenting.FunctionComment.Missing rule is currently excluded in phpcs.xml.dist.

#3212066: Fix Drupal.Commenting.FunctionComment.Missing is the open issue to work on this, but there are probably hundreds if not thousands of functions/methods that need a docblock adding, and many of those are not inheritdoc so someone has to write a sentence for each. Maybe rescoping this to add docblocks to all setUp() methods would be a good start.

I'll review this issue.

I tested the patch, everything seems to work well.

I think the chosen scope makes sense.

Looks like it needs a reroll. It might also need different versions for different branches, so I'd check to see whether the same patch applies to 10.0.x and 9.4.x.

Large cleanups like this are often scheduled as beta targets because they fall out of sync with HEAD so quickly. That way we ensure they're less disruptive to other patches and also get committer attention right away so they don't have to be rerolled so many times while awaiting commit. The beta phase for 9.4 will begin on May 23. @Niklan, if you're available around then, we could schedule recreating this patch for some time that week? Thanks!

@marciaibanez, there is nothing to test with this patch and nothing that would be "working well". Posting that as a review makes it look like no review actually happened.

To get contribution credit for reviewing an issue like this, I would expect to see the reviewer document what references they looked at to confirm that this is the correct fix, how they checked to ensure all the added docblocks were only for test setUp() methods, and how they checked to make sure there were no typos. I would also expect them to show the grep or similar commands they used to make sure no other setUp()methods in HEAD were still missing docblocks once the patch is applied.

So, I haven't assigned @marciaibanez contribution credit for this issue, but hopefully this will help with reviewing similar issues in the future.

Thanks everyone!

Adding tag.

Search pattern:

^$
  protected function setUp\(\): void \{

This is not sufficient. Drupal did not consistently use the :void return type for the setUp() method in Drupal 9 - that was not fully fixed until Drupal 10.

So your pattern doesn't find all the places that need a docblock. For example, core/modules/content_translation/tests/src/Functional/ContentTranslationTestBase.php
and
core/modules/image/tests/src/Functional/ImageFieldTestBase.php
and
core/modules/serialization/tests/src/Kernel/NormalizerTestBase.php
etc.

This means the patch is going to need a lot of changes for Drupal 10.

IMO this should be done in Drupal 10 first, then backported to Drupal 9 if desired. I would hate to see the effort wasted on fixing this in Drupal 9 and not getting the fixes into Drupal 10 ...

Adding a date tag and postponing so we know when to work on this again.

> IMO this should be done in Drupal 10 first

Yes, we will need separate patches for 10.0.x and 9.4.x, with the intent to commit them at the same time.

The patches in #17 look good, but a few more things are needed for the D9 patch.

I forgot to mention that in Drupal 9, in addition to the missing :void, many of the setUp() methods are still declared public instead of protected. So there are some public function setUp() and some public function setUp(): void methods that also need a docblock. Again, the visibility of the setUp() method has been fixed in D10, so this only affects the D9 patch.

I'm wondering if we should address tearDown() at the same time? We usually do. There are only a few of these, but they are strongly correlated with the setUp() changes so it will help minimize disruption if we do them at the same time. (If a class has a tearDown() (not many do) and if that tearDown() is missing a docblock, the setUp() method in that class is almost certainly also missing a docblock. So the classes that need a change to tearDown() will be the same as the classes that need a change to setUp().)

Searching for tearDown() in D9 has the same problems as setUp() - some are public, some are protected, some have :void, some don't.

@Niklan, hope you don't mind that I rerolled this. I was looking at coding standards issues today and discovered this needed a reroll.

I did not search for any new instances that need this change.

Thanks for rerolling.

+++ b/core/modules/views/tests/src/Functional/ViewTestBase.php
@@ -29,6 +29,14 @@ abstract class ViewTestBase extends BrowserTestBase {
+  /**
+   * {@inheritdoc}
+   *
+   * @param bool $import_test_views
+   *   Indicates whether test views configuration should be imported.
+   * @param array $modules
+   *   An array with modules from which views configurations should be imported.
+   */

This is not allowed according to standards, you can either use @inheritdoc alone or a full docblock, but not combine the two. See #1994890: Allow {@inheritdoc} and additional documentation for history and discussion to change this.

I think @inheritdoc will be enough here, so I have made changes as per that.

This patch was neither RTBC nor passing tests. So moving this back to 9.4.x for a commit now seems imprudent, since it's not in a committable state.

Hence moving this to the next beta target phase. https://www.drupal.org/about/core/policies/core-release-cycles/schedule says May 18-27 is the 9.4.x beta phase. The 9.5.x equivalent is Sep 12.

Yes, I know but I felt like getting this back to green. Let's see if this works.

Hehehe … 😄

Who knows 🤞

The last test here ran into a random JS fail, queued it again.

Needs some rerolls here. Neither patch applies to 9.5.x or 10.0.x

@Niklan, thanks for keeping this up to date!

The patches apply to 9.5 and 10.0 but not 10.1. Setting to NW for a 10.1 patch.

@Niklan, thanks!

Removing re-roll tag.

Thank you for continuing to work on this. However, there are lots of files still missing docblocks for setUp(). I generated a list with:

$ rg -il 'function setUp\(' core|xargs rg -i --files-without-match --multiline '\*/\n.*function setUp\('|sort
core/modules/block_content/tests/src/Functional/Views/BlockContentTestBase.php
core/modules/comment/tests/src/Functional/Views/CommentFieldFilterTest.php
core/modules/comment/tests/src/Functional/Views/CommentTestBase.php
core/modules/comment/tests/src/Functional/Views/DefaultViewRecentCommentsTest.php
core/modules/comment/tests/src/Kernel/Views/CommentViewsKernelTestBase.php
core/modules/config_translation/tests/src/Functional/ConfigTranslationViewListUiTest.php
core/modules/field/tests/src/Functional/Views/FieldTestBase.php
core/modules/forum/tests/src/Functional/Views/ForumIntegrationTest.php
core/modules/hal/tests/src/Functional/comment/Views/CommentHalExportTest.php
core/modules/hal/tests/src/Functional/rest/Views/StyleSerializerTest.php
core/modules/image/tests/src/Kernel/Views/RelationshipUserImageDataTest.php
core/modules/language/tests/src/Kernel/Views/LanguageTestBase.php
core/modules/options/tests/src/Kernel/Views/OptionsTestBase.php
core/modules/rest/tests/src/Functional/Views/StyleSerializerTest.php
core/modules/statistics/tests/src/Functional/Views/IntegrationTest.php
core/modules/taxonomy/tests/src/Functional/Views/TaxonomyFieldFilterTest.php
core/modules/user/tests/src/Functional/Views/FilterPermissionUiTest.php
core/modules/user/tests/src/Functional/Views/HandlerFilterUserNameTest.php
core/modules/user/tests/src/Functional/Views/UserChangedTest.php
core/modules/user/tests/src/Functional/Views/UserTestBase.php
core/modules/user/tests/src/Kernel/Views/UserKernelTestBase.php
core/modules/views/tests/src/Functional/DefaultViewsTest.php
core/modules/views/tests/src/Functional/Handler/AreaTest.php
core/modules/views/tests/src/Functional/Handler/FieldEntityOperationsTest.php
core/modules/views/tests/src/Functional/Handler/FieldWebTest.php
core/modules/views/tests/src/Functional/Handler/FilterDateTest.php
core/modules/views/tests/src/Functional/Handler/HandlerTest.php
core/modules/views/tests/src/Functional/Plugin/AccessTest.php
core/modules/views/tests/src/Functional/Plugin/ArgumentDefaultTest.php
core/modules/views/tests/src/Functional/Plugin/CacheTagTest.php
core/modules/views/tests/src/Functional/Plugin/DisabledDisplayTest.php
core/modules/views/tests/src/Functional/Plugin/DisplayAttachmentTest.php
core/modules/views/tests/src/Functional/Plugin/DisplayFeedTest.php
core/modules/views/tests/src/Functional/Plugin/DisplayFeedTranslationTest.php
core/modules/views/tests/src/Functional/Plugin/DisplayTest.php
core/modules/views/tests/src/Functional/Plugin/ExposedFormTest.php
core/modules/views/tests/src/Functional/Plugin/FilterTest.php
core/modules/views/tests/src/Functional/Plugin/MiniPagerTest.php
core/modules/views/tests/src/Functional/Plugin/NumericFormatPluralTest.php
core/modules/views/tests/src/Functional/Plugin/ViewsBulkTest.php
core/modules/views/tests/src/Functional/TaxonomyGlossaryTest.php
core/modules/views/tests/src/Functional/ViewAjaxTest.php
core/modules/views/tests/src/Functional/ViewTestBase.php
core/modules/views/tests/src/Functional/Wizard/BasicTest.php
core/modules/views/tests/src/Functional/Wizard/ItemsPerPageTest.php
core/modules/views/tests/src/Functional/Wizard/SortingTest.php
core/modules/views/tests/src/Functional/Wizard/TaggedWithTest.php
core/modules/views/tests/src/Functional/Wizard/WizardTestBase.php
core/modules/views/tests/src/Kernel/Handler/AreaTextTest.php
core/modules/views/tests/src/Kernel/Handler/HandlerAliasTest.php
core/modules/views/tests/src/Kernel/PluginInstanceTest.php
core/modules/views/tests/src/Kernel/Plugin/JoinTest.php
core/modules/views/tests/src/Kernel/ViewsHooksTest.php
core/modules/views/tests/src/Kernel/Wizard/WizardPluginBaseKernelTest.php
core/modules/views_ui/tests/src/Functional/DefaultViewsTest.php
core/modules/views_ui/tests/src/Functional/DisplayPathTest.php
core/modules/views_ui/tests/src/Functional/DuplicateTest.php
core/modules/views_ui/tests/src/Functional/ExposedFormUITest.php
core/modules/views_ui/tests/src/Functional/OverrideDisplaysTest.php
core/modules/views_ui/tests/src/Functional/TranslatedViewTest.php

DCOM58Test was copied from Doctrine and is fine to leave as-is I think.

For the tests with parameters I guess we should document the base classes correctly, but any subclasses that have the same parameters should use @inheritdoc. I will double check this later.

I reviewed the whole 9.5.x patch as that is the largest (some modules have been removed in 10.0.x). The only issue I found was:

+++ b/core/modules/views/tests/src/Functional/ViewTestBase.php
@@ -29,6 +29,9 @@ abstract class ViewTestBase extends BrowserTestBase {
+  /**
+   * {@inheritdoc}
+   */
   protected function setUp($import_test_views = TRUE, $modules = ['views_test_config']) {

This is the only docblock where new parameters are introduced so this cannot use @inheritdoc. I suggest the docblock is something like:

  /**
   * Sets up the test.
   *
   * @param bool $import_test_views
   *   Should the views specified on the test class be imported. If you need
   *   to setup some additional stuff, like fields, you need to call false and
   *   then call createTestViews for your own.
   * @param array $modules
   *   The module directories to look in for test views.
   */

The docs are borrowed from ViewsKernelTestBase which already has a docblock.

All others in this patch are fine to use @inheritdoc as they extend from other base classes.

Thank you for working on this. I checked the patches for all three branches and can't find any missing docblocks, so this issue is RTBC.

Committed 2c468a0 and pushed to 10.1.x. Thanks!
Committed c5b049b and pushed to 10.0.x. Thanks!
Committed 1f60cd2 and pushed to 9.5.x. Thanks!

Backported to 9.5.x to keep things aligned - use --3way to apply all the patch successfully.