[May 2024] Add void return typehints to all test methods

We should be able to use AddVoidReturnTypeWhereNoReturnRector for this, I'll give it a try.

There may also be a phpcs sniff we can use instead which would allow us to add to the existing ruleset.

Also found VoidReturnFixer for PHP-CS-Fixer but nothing yet for phpcs.

Added first attempt of an MR generated with PHP-CS-Fixer, added steps to reproduce.

[ERROR] Found 121 errors

Have resolved all the phpstan issues. Here's the diff of changes since running php-cs-fixer.

We now have a small number of errors and test failures to resolve.

Possibly we should have attempted to limit this to functions with the test prefix but I'm not sure that's possible to automate. Tests are passing now, so this is over for review and deliberation about how this could be handled or, dare I say, split.

I scanned majority of the MR (up to image module). 90-95% of the changes are just test case functions. The remaining are things like setUpContentType, anonymous/inline callbacks and doAssert style functions. If it's not automatable to restrict to just test cases I'd vote for just including everything in all test files to avoid a lot of manual work.

This could be split up, but I won't comment on if it should or not :D

I also separately reviewed https://git.drupalcode.org/issue/drupal-3421418/-/compare/981d4f6368d4ff... and those changes make sense to me.

However, I'm not sure why we can't add it to EntityResourceTestBase::testPost?

For me this looks good to go, nice work @mstrelan

Thanks for the review. Have fixed EntityResourceTestBase::testPost. For some reason \Drupal\Tests\entity_test\Functional\Rest\EntityTestComputedFieldNormalizerTest::testPost did not initially have void added and that made phpstan sad. Instead I've just added void to both methods.

Nice, I'm gonna mark RTBC to see if anyone thinks it should be split up :)

I've found a way to apply this only to test* methods by using rector and hacking the \Rector\TypeDeclaration\Rector\ClassMethod\AddVoidReturnTypeWhereNoReturnRector::shouldSkipClassMethod method with this:

if (!\str_starts_with($classMethod->name->name, 'test')) {
    return \true;
}

Then using a rector.php file:

<?php

return Rector\Config\RectorConfig::configure()
  ->withRules([Rector\TypeDeclaration\Rector\ClassMethod\AddVoidReturnTypeWhereNoReturnRector::class])
  ->withPaths(['core/tests', 'core/*/*/tests'])
;

And finally:

./vendor/bin/rector

Have created a separate MR for this so we can discuss which route we want to take here.

Updated IS with the two approaches and remaining tasks

Both MRs appear to be unmergable.

As much as I like the all in one, this could touch 1000s of files and be difficult to review. Should it be broken up?

Thanks for the work so far!

One big megapatch, in an agreed date during the beta window.

Correct, this is non critical cleanup, so to avoid disrupting other issues we are better off doing this in one hit, but we have to time it for least disruption. It's a bit tricky because release dates are up in the air, depending on when we complete Drupal 11 beta requirements: https://www.drupal.org/about/core/policies/core-release-cycles/schedule

May 2024 covers all three windows so I think a vague date of that is worth adding for now, as we get closer to the time we can revive this and get it ready to commit again.

Added to #3284879: [meta] Requirements for tagging Drupal 11.0.0-beta1 so we have less chance of forgetting.

Assuming the current plan would be to work on this after the alpha. We should probably also postpone this on #3427999: [PHP 8.4] Fix implicitly nullable type declarations since that will touch a lot of the same methods.

Well it's May 2024 so maybe we should pick this back up. I've updated the IS, rebased MR 6606 and hidden 6604. Unless we want to postpone this on #3427999: [PHP 8.4] Fix implicitly nullable type declarations then this is ready for review.

1000+ files is hard to review and crashes gitlab

But the 1 to 1 code addition to subtraction does indicate nothing more was added.

Been trying to help tickets I review by adding :void to tests (nitpicky stuff) but still large number here.

Spot checking the files I don't see anything off and tests are still green.

Believe this is good.

Yeah for these massive diffs I find it easier to review the raw diff than the formatted one. You could probably also grep and wc -l for the numbers of lines with ) { and the number with ): void {. Can also grep those to see if they start with - function test and + function test

Had to rebase again. I then put together the following commands to review the changes:

Generate patch with 0 lines of context, excluding all the git noise.

git diff -U0 11.x | grep -Ev '^(diff --git|index|--- a/|\+\+\+ b/|@@)' > 3421418.patch

Count the lines of the patch adding :void to the end of a test function and ensure it matches the number of lines removing a test function.

REGEX_ADD='\+\s+public function test.*: void'
REGEX_REMOVAL='\-\s+public function test.*$'

$ grep -E "$REGEX_ADD" 3421418.patch | wc -l
8466

$ grep -E "$REGEX_REMOVAL" 3421418.patch | wc -l
8466

Find any remaining differences:

grep -Ev "$REGEX_ADD|$REGEX_REMOVAL" 3421418.patch
-    return parent::testPatchIndividual();
+    parent::testPatchIndividual();
+// cspell:ignore yarhar
+
-    public function   testSyntaxErrorWithUnknownCharacters()
+    public function   testSyntaxErrorWithUnknownCharacters(): void

These changes are found in the following places:

• \Drupal\Tests\jsonapi\Functional\CommentTest::testPatchIndividual - can't return a void function
• \Drupal\FunctionalJavascriptTests\Ajax\AjaxTest - not sure why this wasn't failing cspell already
• \Drupal\Tests\Component\Annotation\Doctrine\DocParserTest::testSyntaxErrorWithUnknownCharacters - maybe we should exclude doctrine annotations here?

Setting NW because I think we should exclude \Drupal\Tests\Component\Annotation\Doctrine.

Rebased, which has removed the need for the cspell ignore, and reverted the change to doctrine tests.

Reran the commands in #25 and end up with the following:

$ grep -Ev "$REGEX_ADD|$REGEX_REMOVAL" 3421418.patch
-    return parent::testPatchIndividual();
+    parent::testPatchIndividual();

Seems to still need a rebase. May be one of those things will need to rebase, immediately review, and have a committer ready

This will need enough MRs to apply to all active branches - 11.x, 11.0.x, 10.4.x and 10.3.x - as we need to commit it everywhere to keep things in sync.

I have hidden all the existing MRs and added new ones from 11.x, 11.0.x, 10.4.x and 10.3.x. Have run the checks in #25 and the only change that isn't just adding : void is changing CommentTest::testPatchIndividual to not return anything.

I began reviewing this with a porcelain diff. PHP script to parse the diff in my home directory:

$file = file('./diff_output.txt');

$expected_line_starts = [
  'diff',
  'index',
  '---',
  '+++',
  '@@',
  '~',
];
foreach ($file as $line) {
  if (!str_starts_with($line, ' ')) {
    $line = trim($line);
    if ($line !== '+: void') {
      $line_parts = explode(' ', $line);
      if (!in_array(trim($line_parts[0]), $expected_line_starts)) {
        print $line . "\n";
      }
    }
  }
}

Steps:

curl https://git.drupalcode.org/project/drupal/-/merge_requests/8442.diff | git apply --index -
git diff --staged --word-diff=porcelain --unified=0 > ~/diff_output.txt
cd ~
php parse-void-porcelain.php 

This flagged exactly one change that isn't exactly adding : void somewhere, which I located to be the following hunk in core/modules/jsonapi/tests/src/Functional/CommentTest.php:

@@ -476,7 +476,7 @@ public function testPatchIndividual() {
       'type' => 'bar',
     ])->save();
 
-    return parent::testPatchIndividual();
+    parent::testPatchIndividual();
   }
 
 }

I suppose this is necessary because without removing the return, it would break the contract of the void return typehint.

Ideally, that change would have been fixed in a separate issue; however, given the scope and urgency of this, and that it's just a one-off, I'll let it slide. :)

Later, I'll make the porcelain-parsing script a little more robust to ensure the added : void is actually a function typehint, but for now, I'll check the other MRs. :)

I confirmed the 11.0.x, 10.4.x, and 10.3.x merge requests also have that one outlying change. Now I'll tweak the script to add a hunk validating that the : void additions are in fact test typehints, and repeat.

Improved script validating that these are all correctly placed void typehints on test methods:

$file = file('./diff_output.txt');

$expected_line_starts = [
  'diff',
  'index',
  '---',
  '+++',
  '@@',
  '~',
];

$errant_lines = [];
foreach ($file as $i => $line) {
  if (!str_starts_with($line, ' ')) {
    $line = trim($line);
    // Flag any word additions or removals that don't look like the expected
    // void return typehint.
    if ($line !== '+: void') {
      $line_parts = explode(' ', $line);
      if (!in_array(trim($line_parts[0]), $expected_line_starts)) {
        $errant_lines[] = $i;
      }
    }
    // Ensure that additions of ': void' do in fact appear to be return
    // typehints.
    else {
      // The context words before should declare a function testSomething().
      if (!str_contains($file[$i - 1], ' function test')) {
        $errant_lines[] = $i;
      }
      // The context words before should end with a closing parenthesis.
      if (!str_ends_with(trim($file[$i - 1]), ')')) {
        $errant_lines[] = $i;
      }
      // The context words after should contain only a curly and whitespace.
      if (!trim($file[$i + 1]) === '{') {
        $errant_lines[] = $i;
      }
      // The line after that should contain a tilde, indicating a line break.
      if (!trim($file[$i + 2]) === '~') {
        $errant_lines[] = $i;
      }
    }
  }
}

foreach ($errant_lines as $i) {
  print $file[$i];
}

Next I'll validate that only test files are changed.

Checked the file list next. Steps after applying the patch:

git diff --name-only --staged > ~/filenames.txt
cd ~
php parse-filenames.php

That script simply being:

$file = file('./filenames.txt');

foreach ($file as $line) {
  if (!str_starts_with($line, 'core/tests') && !str_contains($line, 'tests/src')) {
    echo $line;
  }
}

This has flagged what I think is actually an out-of-scope change?

diff --git a/core/modules/system/tests/modules/router_test_directory/src/TestControllers.php b/core/modules/system/tests/modules/router_test_directory/src/TestControllers.php
index 605ef69d5a..e03cd90296 100644
--- a/core/modules/system/tests/modules/router_test_directory/src/TestControllers.php
+++ b/core/modules/system/tests/modules/router_test_directory/src/TestControllers.php
@@ -72,7 +72,7 @@ public function test9($uid) {
    * Passes unsafe HTML as an argument to a method which throws an exception.
    * This can be used to test if the generated backtrace is properly escaped.
    */
-  public function test10() {
+  public function test10(): void {
     $this->removeExceptionLogger();
     $this->throwException('<script>alert(\'xss\')</script>');
   }
@@ -98,7 +98,7 @@ public function test23() {
     return new Psr7Response(200, [], 'test23');
   }
 
-  public function test24() {
+  public function test24(): void {
     $this->removeExceptionLogger();
     throw new \Exception('Escaped content: <p> <br> <h3>');
   }

TestControllers appears to be a test fixture used by various routing system tests that just has some really terribly named route callbacks. For example, the above test24() is used in
core/modules/system/tests/modules/router_test_directory/router_test.routing.yml:

router_test.24:
  path: '/router_test/test24'
  defaults:
    _controller: '\Drupal\router_test\TestControllers::test24'
  requirements:
    _access: 'TRUE'

This is used in core/tests/Drupal/KernelTests/Core/Routing/ExceptionHandlingTest.php:

  /**                                                                           
   * Tests exception message escaping.                                          
   */
  public function testExceptionEscaping(): void {
    // Enable verbose error logging.                                            
    $this->config('system.logging')->set('error_level', ERROR_REPORTING_DISPLAY_VERBOSE)->save();

    // Using \Drupal\Component\Render\FormattableMarkup.                        
    $request = Request::create('/router_test/test24');
    $request->setFormat('html', ['text/html']);

    // Etc. etc. etc.

So it's not a test method at all.

That said, it's not wrong for it to have a void typehint. It does, after all, return void. But did this get lumped in because of greppiness, or because of static analysis? If the latter, we might have a problem. If the former, let's just remove those two hunks from the MRs.

Tagging "Needs followup" to fix the absolutely terrible routing test fixture naming.

Reverted the changes to TestControllers and opened #3455287: Rename \Drupal\router_test\TestControllers and its methods. Removing tag for follow up.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. 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.

Going to remark as tests are green and these seem like MRs that will always have conflicts so may be nice to get this chunk in. But testControllers appear reverted.

I was only halfway through my review. I'd addressed that the MR didn't contain anything it shouldn't, but not that it did contain everything it should. With a very large MR like this, there are a lot of strategies to use (the simplest being a local git diff --staged --color-words for example), but we should use some of them because we need our peer reviewers to help catch issues. Simply verifying that all a committer's feedback has been addressed isn't really sufficient on its own for an issue to be RTBC, unless everything else already was RTBC with actual peer review, and it wasn't here.

Setting NR, and I'll continue to work on it. Thanks!

I rechecked the updated MRs with #41 - #44, and confirmed that the issue with the routing controller is resolved without any additional malformed changes being introduced.

Next going to do some grepping to look for stragglers.

Next, I reviewed all places where a function with a name starting with "test" in the core tests (not module tests yet) was missing a return typehint. My grep was pretty basic to cast a wide net:
grep -r "function test" core/tests/* | grep ") {" > ~/tmp.txt
(Then deduplicated and parsed out the filenames from that file using emacs macros, and opened all the listed files to look at the matches in context.)

Three categories of thing:

Stuff that looks to be fixtures

1. fixtures/test_stable/test_stable.theme

2. Drupal/Tests/Core/Form/fixtures/form_base_test.inc

3. Drupal/Tests/Core/Database/Stub/StubConnection.php

4. Fixture global function being written out to the filesystem:

     /**                                                                           
      * {@inheritdoc}                                                              
      */
     protected function visitInstaller() {
       // Create an .install file with a hook_install() implementation.            
       $path = $this->siteDirectory . '/profiles/' . $this->profile;
       $contents = <<<EOF
   <?php
   
   function testing_config_install_multilingual_install() {
   }
   EOF;
       file_put_contents("$path/{$this->profile}.install", $contents);
       parent::visitInstaller();
     }
   

5. /**                                                                             
    * Provides a different test interface.                                         
    */
   interface Test2Interface {
   }
   
   function test_access_arguments_resolver_access($foo) {
   }

6. Global functions I assume to be fixtures added outside the test class in EntityResolverManagerTest:

   function test_function_controller() {
   }
   
   function test_function_controller_with_argument($argument) {
   }
   
   function test_function_controller_no_upcasting($entity_test) {
   }
   
   function test_function_controller_entity_upcasting(EntityInterface $entity_test) {
   }
   

Stuff that actually has return values

Should we have a followup to research test methods with return values and determine if they're intentional or not?

1. In ProxyBuilderTest:

   /**                                                                             
    * {@inheritdoc}                                                                
    */
   public function testMethod($parameter)
   {
       return $this->lazyLoadItself()->testMethod($parameter);
   }
   

   Returns a thing (might merit a closer looksee).

2. In UrlTest:

    /**                                                                           
      * Tests creating a URL from a request.                                       
      */
     public function testUrlFromRequest() {
   

   It returns $urls.

3. In KeyValueEntityStorageTest and ConfigEntityStorageTest there are some identical methods with return values:

     /**                                                                           
      * @covers ::create                                                           
      * @covers ::doCreate                                                         
      *                                                                            
      * @return \Drupal\Core\Entity\EntityInterface                                
      */
     public function testCreate() {
   

     /**                                                                           
      * @covers ::save                                                             
      * @covers ::doSave                                                           
      *                                                                            
      * @param \Drupal\Core\Entity\EntityInterface $entity                         
      *   The entity.                                                              
      *                                                                            
      * @return \Drupal\Core\Entity\EntityInterface                                
      *                                                                            
      * @depends testCreate                                                        
      */
     public function testSaveInsert(EntityInterface $entity) {
   

     /**                                                                           
      * @covers ::save                                                             
      * @covers ::doSave                                                           
      *                                                                            
      * @param \Drupal\Core\Entity\EntityInterface $entity                         
      *   The entity.                                                              
      *                                                                            
      * @return \Drupal\Core\Entity\EntityInterface                                
      *                                                                            
      * @depends testSaveInsert                                                    
      */
     public function testSaveUpdate(EntityInterface $entity) {
   

     /**                                                                           
      * @covers ::save                                                             
      * @covers ::doSave                                                           
      */
     public function testSaveConfigEntity() {
   

   All return the entity.

4. In FormStateTest:

     /**                                                                           
      * @covers ::addCleanValueKey                                                 
      */
     public function testAddCleanValueKey() {
       $form_state = new FormState();
       $form_state->setValue('value_to_clean', 'rainbow_sprinkles');
       $form_state->addCleanValueKey('value_to_clean');
       $this->assertSame($form_state->getCleanValueKeys(), ['form_id', 'form_token', 'form_build_id', 'op', 'value_to_clean']);
       return $form_state;
     }
   

Other

1. In StaticReflectionParserTest:

     public function testAttribute(string $class, string $attribute_class, bool $expected) {
       $finder = MockFileFinder::create(__DIR__ . '/Fixtures/Attribute/' . $class . '.php');
       $parser = new StaticReflectionParser('\\Drupal\\Tests\\Component\\Annotation\\Doctrine\\Fixtures\\Attribute\\' . $class, $finder);
       $this->assertSame($expected, $parser->hasClassAttribute($attribute_class), "'$class' has attribute that is a '$attribute_class'");
     }
   

   Excluded for being the Doctrine fork, I guess.

2. I don't totally understand what this is, but it's in ExternalCommandRequirementTest.

   class MethodRequires {
   
     use ExternalCommandRequirementsTrait;
   
     /**                                                                           
      * @requires externalCommand available_command                                
      */
     public function testRequiresAvailable() {
   
     }
   
     /**                                                                           
      * @requires externalCommand unavailable_command                              
      */
     public function testRequiresUnavailable() {
   
     }
   
     protected static function externalCommandIsAvailable($command) {
       return in_array($command, ['available_command']);
     }
   
   }
   

3. In UpdatePathTestBase:

     /**                                                                           
      * Tests that the database was properly loaded.                               
      */
     protected function testDatabaseLoaded() {
   

   This doesn't actually seem to return anything. I notice that the method is protected. Not sure what's going on with this one.

The list from module tests is fortunately shorter.

Fixtures

1. Outside the actual test class in PathPluginBaseTest:

   /**                                                                             
    * A page controller for use by tests in this file.                             
    */
   class TestController {
   
     /**                                                                           
      * A page title callback.                                                     
      *                                                                            
      * @return string                                                             
      *   The page title.                                                          
      */
     public function testTitle() {
       return 'Test title';
     }
   
   }
   

Things that actually return stuff

1. Both DateFormatAccessControlHandlerTest and MenuAccessControlHandlerTest have an undocumented test method which returns a massive access result array:

   public static function testAccessProvider() {

   Looking more closely, this is just a mis-named data provider for the properly typehinted testAccess() method, so merits its own issue to fix.

2. In JsonApiDocumentTopLevelNormalizerTest:

     /**                                                                           
      * Provides test cases for asserting cacheable metadata behavior.             
      */
     public static function testCacheableMetadataProvider() {
       $cacheable_metadata = function ($metadata) {
         return CacheableMetadata::createFromRenderArray(['#cache' => $metadata]); 
       };
   
       return [
         [
           $cacheable_metadata(['contexts' => ['languages:language_interface']]),
           ['node--article' => 'body'],
         ],
       ];
     }
   

   This one is another mis-named data provider.

Other

1. Commented-out code in ArgumentDefaultTest:

     /**                                                                           
      * @todo Test php default argument.                                           
      */
     // function testArgumentDefaultPhp() {}   
   

   (A followup should be filed, either to add the test or just to remove the issue-less @todo.)

2. In FileUploadTest:

     /**                                                                           
      * Tests using the file upload POST route with invalid headers.               
      */
     protected function testPostFileUploadInvalidHeaders() {
   

     /**                                                                           
      * Tests using the file upload route with an invalid file type.               
      */
     protected function testFileUploadInvalidFileType() {
   

     /**                                                                           
      * Tests using the file upload route with a file size larger than allowed.    
      */
     protected function testFileUploadLargerFileSize() {
   

     /**                                                                           
      * Tests using the file upload route with a file size larger than allowed.    
      */
     protected function testFileUploadLargerFileSize() {
   

     /**                                                                           
      * Tests using the file upload POST route with malicious extensions.          
      */
     protected function testFileUploadMaliciousExtension() {
   

   None of these actually return anything. They are protected methods. They're called as helpers from e.g. testInvalidFileUploads().

For completeness' sake, I'll also look through theme and profile tests etc., then sort through what needs followups vs. things we might want to look at here.

core/profiles/demo_umami/tests/src/Functional/DemoUmamiProfileTest.php has another protected helper that was not included but does return void:

  /**                                                                           
   * Tests demo_umami profile warnings shown on Status Page.                    
   */
  protected function testWarningsOnStatusPage() {
    $account = $this->drupalCreateUser(['administer site configuration']);
    $this->drupalLogin($account);

    // Check the requirements warning for using an experimental profile.        
    $this->drupalGet('admin/reports/status');
    $this->assertSession()->pageTextContains('Experimental profiles are provide\
d for testing purposes only. Use at your own risk. To start building a new site\
, reinstall Drupal and choose a non-experimental profile.');
  }

OK, based on the above, here's what I recommend:

1. One followup to add return typehints to protected test helper methods (and possibly change their naming pattern when they're named testSomething() despite being protected; the usual pattern is assertSomething()).
2. One followup to rename the various mis-named data providers.
3. One followup to audit public test methods that return something and validate that they're in fact supposed to.
4. One followup to add a proper to-do for the commented-out test method, and/or to just remove it.

For this issue, I'd like to understand these two things before commit:

• I don't totally understand what this is, but it's in ExternalCommandRequirementTest.

  class MethodRequires {
  
    use ExternalCommandRequirementsTrait;
  
    /**                                                                           
     * @requires externalCommand available_command                                
     */
    public function testRequiresAvailable() {
  
    }
  
    /**                                                                           
     * @requires externalCommand unavailable_command                              
     */
    public function testRequiresUnavailable() {
  
    }
  
    protected static function externalCommandIsAvailable($command) {
      return in_array($command, ['available_command']);
    }
  
  }
  

• In UpdatePathTestBase:

    /**                                                                           
     * Tests that the database was properly loaded.                               
     */
    protected function testDatabaseLoaded() {
  

  This doesn't actually seem to return anything. I notice that the method is protected. Not sure what's going on with this one.

I'll get to work on the followups, starting with the first one,

That's all the proposed followups filed.

Re: #55

ExternalCommandRequirementTest was removed from 11.x in #3425660: [11.x] Remove deprecated ExternalCommandRequirementsTrait so I think we can leave it as it is in 10.3 and 10.4

UpdatePathTestBase::testDatabaseLoaded is a protected helper method called from UpdatePathTestBaseFilledTest and UpdatePathTestBaseTest so I think it should be addressed in #3455549: Add return typehints to protected test helper methods.

I re-reviewed the 11.x MR with the same steps as I used in #51 - #54 with the 10.3.x MR, and confirmed that there are no additional weirdnesses aside from things we've already explained and/or scoped to followups.

So I think it's RTBC!

Committed/pushed to 11.x, 11.0.x, 10.4.x, 10.3.x respectively thanks!

I could not get commit checks to actually complete on commit, so ended up committing all four patches with --no-verify. Given this doesn't yet introduce the phpcs rule, that ought to be OK, hopefully we don't find out it wasn't.