PHPUnit tests should have a return type

moodle/Sniffs/PHPUnit/TestReturnTypeSniff.php

@@ -0,0 +1,156 @@
+<?php
+// This file is part of Moodle - http://moodle.org/
+//
+// Moodle is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// Moodle is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
+
+namespace MoodleHQ\MoodleCS\moodle\Sniffs\PHPUnit;
+
+// phpcs:disable moodle.NamingConventions
+
+use MoodleHQ\MoodleCS\moodle\Util\MoodleUtil;
+use PHP_CodeSniffer\Sniffs\Sniff;
+use PHP_CodeSniffer\Files\File;
+use PHPCSUtils\Utils\FunctionDeclarations;
+use PHPCSUtils\Utils\ObjectDeclarations;
+
+/**
+ * Checks that a test file has the @coversxxx annotations properly defined.
+ *
+ * @package    local_codechecker
+ * @copyright  2022 onwards Eloy Lafuente (stronk7) {@link https://stronk7.com}
+ * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+class TestReturnTypeSniff implements Sniff
+{
+
+    /**
+     * Register for open tag (only process once per file).
+     */
+    public function register() {
+        return [
+            T_OPEN_TAG,
+        ];
+    }
+
+    /**
+     * Processes php files and perform various checks with file.
+     *
+     * @param File $file The file being scanned.
+     * @param int $pointer The position in the stack.
+     */
+    public function process(File $file, $pointer)
+    {
+        // Before starting any check, let's look for various things.
+
+        // If we aren't checking Moodle 4.0dev (400) and up, nothing to check.
+        // Make and exception for codechecker phpunit tests, so they are run always.
+        if (!MoodleUtil::meetsMinimumMoodleVersion($file, 400) && !MoodleUtil::isUnitTestRunning()) {
+            return; // @codeCoverageIgnore
+        }
+
+        // If the file is not a unit test file, nothing to check.
+        if (!MoodleUtil::isUnitTest($file) && !MoodleUtil::isUnitTestRunning()) {
+            return; // @codeCoverageIgnore
+        }
+
+        // We have all we need from core, let's start processing the file.
+
+        // Get the file tokens, for ease of use.
+        $tokens = $file->getTokens();
+
+        // We only want to do this once per file.
+        $prevopentag = $file->findPrevious(T_OPEN_TAG,
+            $pointer - 1
+        );
+        if ($prevopentag !== false) {
+            return; // @codeCoverageIgnore
+        }
+
+        // Iterate over all the classes (hopefully only one, but that's not this sniff problem).
+        $cStart = $pointer;
+        while ($cStart = $file->findNext(T_CLASS, $cStart + 1)) {
+            if (MoodleUtil::isUnitTestCaseClass($file, $cStart) === false) {
+                // This class does not related to a uit test.
+                continue;
+            }
+
+            $classInfo = ObjectDeclarations::getClassProperties($file, $cStart);
+
+            // Iterate over all the methods in the class.
+            $mStart = $cStart;
+            while ($mStart = $file->findNext(T_FUNCTION, $mStart + 1, $tokens[$cStart]['scope_closer'])) {
+                $method = $file->getDeclarationName($mStart);
+
+                // Ignore non test_xxxx() methods.
+                if (strpos($method, 'test_') !== 0) {
+                    continue;
+                }
+
+                // Get the function declaration.
+                $functionInfo = FunctionDeclarations::getProperties($file, $mStart);
+
+                // 'return_type_token'     => int|false, // The stack pointer to the start of the return type.
+                if ($functionInfo['return_type_token'] !== false) {
+                    // This method has a return type.
+                    // In most cases that will be void, but it could be anything in the
+                    // case of chained or dependant tests.
+                    // TODO: Detect all cases of chained tests and dependant tests.
+                    continue;
+                }
+
+                $methodNamePtr = $file->findNext(T_STRING, $mStart + 1, $tokens[$mStart]['parenthesis_opener']);
+                $methodEnd = $file->findNext(T_CLOSE_PARENTHESIS, $mStart + 1);
+
+                // Detect if the method has a return statement.
+                // If it does, then see if it has a value or not.
+                $hasReturn = $file->findNext(T_RETURN, $mStart + 1, $tokens[$mStart]['scope_closer']);
+                $probablyVoid = !$hasReturn;
+                if ($hasReturn) {
+                    $next = $file->findNext(
+                        T_WHITESPACE,
+                        $hasReturn + 1,
+                        $tokens[$mStart]['scope_closer'],
+                        true
+                    );
+                    if ($tokens[$next]['code'] === T_SEMICOLON) {
+                        $probablyVoid = true;
+                    }
+                }
+
+                $fix = false;
+                if ($probablyVoid) {
+                    $fix = $file->addFixableWarning(
+                        'Test method %s() is missing a return type',
+                        $methodNamePtr,
+                        'MissingReturnType',
+                        [$method]
+                    );
+                } else {
+                    $file->addWarning(
+                        'Test method %s() is missing a return type',
+                        $methodNamePtr,
+                        'MissingReturnType',
+                        [$method]
+                    );
+                }
+
+                if ($fix) {
+                    $file->fixer->beginChangeset();
+                    $file->fixer->addContent($methodEnd, ': void');
+                    $file->fixer->endChangeset();
+                }
+            }
+        }
+    }
+}

moodle/Tests/MoodleUtilTest.php

@@ -22,6 +22,7 @@
 use PHP_CodeSniffer\Files\File;
 use PHP_CodeSniffer\Ruleset;
 use org\bovigo\vfs\vfsStream;
+use PHPCSUtils\Utils\ObjectDeclarations;
 
 // phpcs:disable moodle.NamingConventions
 
@@ -470,7 +471,7 @@ protected function cleanMoodleUtilCaches() {
     /**
      * Data provider for testIsUnitTest.
      *
-     * @return array 
+     * @return array
      */
     public static function isUnitTestProvider(): array
     {
@@ -529,6 +530,74 @@ public function testIsUnitTest(
         $this->assertEquals($expected, MoodleUtil::isUnitTest($file));
     }
 
+    /**
+     * Data provider for testIsUnitTest.
+     *
+     * @return array
+     */
+    public static function isUnitTestCaseClassProvider(): array {
+        return [
+            'Not in tests directory' => [
+                'value' => '/path/to/standard/file_test.php',
+                'testClassName' => 'irrelevant',
+                'return' => false,
+            ],
+            'testcase in a test file' => [
+                'value' => __DIR__ . '/fixtures/moodleutil/istestcaseclass/multiple_classes_test.php',
+                'testClassName' => 'is_testcase',
+                'return' => true,
+            ],
+            'not a testcase in a test file' => [
+                'value' => __DIR__ . '/fixtures/moodleutil/istestcaseclass/multiple_classes_test.php',
+                'testClassName' => 'not_a_test_class',
+                'return' => false,
+            ],
+            'not a testcase with test in the name in a test file' => [
+                'value' => __DIR__ . '/fixtures/moodleutil/istestcaseclass/multiple_classes_test.php',
+                'testClassName' => 'some_other_class_with_test_in_name',
+                'return' => false,
+            ],
+            'not a testcase but extends a test class in a test file' => [
+                'value' => __DIR__ . '/fixtures/moodleutil/istestcaseclass/multiple_classes_test.php',
+                'testClassName' => 'some_other_class_with_test_in_name',
+                'return' => false,
+            ],
+            'looks like a test but does not extend a test class' => [
+                'value' => __DIR__ . '/fixtures/moodleutil/istestcaseclass/multiple_classes_test.php',
+                'testClassName' => 'some_other_class_with_test_in_name_not_extending_test',
+                'return' => false,
+            ],
+        ];
+    }
+
+    /**
+     * @dataProvider isUnitTestCaseClassProvider
+     */
+    public function testIsUnitTestCaseClass(
+        string $filepath,
+        string $testClassName,
+        bool $expected
+    ): void
+    {
+        $phpcsConfig = new Config();
+        $phpcsRuleset = new Ruleset($phpcsConfig);
+
+        if (file_exists($filepath)) {
+            $file = new \PHP_CodeSniffer\Files\LocalFile($filepath, $phpcsRuleset, $phpcsConfig);
+            $file->process();
+        } else {
+            $file = new File($filepath, $phpcsRuleset, $phpcsConfig);
+        }
+
+        $cStart = 0;
+        while ($cStart = $file->findNext(T_CLASS, $cStart + 1)) {
+            if (ObjectDeclarations::getName($file, $cStart) === $testClassName) {
+                $this->assertEquals($expected, MoodleUtil::isUnitTestCaseClass($file, $cStart));
+                break;
+            }
+        }
+    }
+
     /**
      * Data provider for testMeetsMinimumMoodleVersion.
      *

moodle/Tests/PHPUnitTestReturnTypeTest.php

@@ -0,0 +1,80 @@
+<?php
+// This file is part of Moodle - http://moodle.org/
+//
+// Moodle is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// Moodle is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
+
+namespace MoodleHQ\MoodleCS\moodle\Tests;
+
+// phpcs:disable moodle.NamingConventions
+
+/**
+ * Test the PHPUnitTestReturnTypeSniff sniff.
+ *
+ * @package    local_codechecker
+ * @category   test
+ * @copyright  2022 onwards Eloy Lafuente (stronk7) {@link http://stronk7.com}
+ * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ *
+ * @covers \MoodleHQ\MoodleCS\moodle\Sniffs\PHPUnit\TestReturnTypeSniff
+ */
+class PHPUnitTestReturnTypeTest extends MoodleCSBaseTestCase {
+
+    /**
+     * Data provider for self::provider_phpunit_data_returntypes
+     */
+    public function provider_phpunit_data_returntypes(): array {
+        return [
+            'Provider Casing' => [
+                'fixture' => 'fixtures/phpunit/TestReturnType/returntypes.php',
+                'errors' => [
+                ],
+                'warnings' => [
+                    6 => 'Test method test_one() is missing a return type',
+                    27 => 'Test method test_with_a_return() is missing a return type',
+                    32 => 'Test method test_with_another_return() is missing a return type',
+                    38 => 'Test method test_with_empty_return() is missing a return type',
+                ],
+            ],
+        ];
+    }
+
+    /**
+     * Test the moodle.PHPUnit.TestCaseCovers sniff
+     *
+     * @param string $fixture relative path to fixture to use.
+     * @param array $errors array of errors expected.
+     * @param array $warnings array of warnings expected.
+     * @dataProvider provider_phpunit_data_returntypes
+     */
+    public function test_phpunit_test_returntypes(
+        string $fixture,
+        array $errors,
+        array $warnings
+    ): void {
+        // Define the standard, sniff and fixture to use.
+        $this->set_standard('moodle');
+        $this->set_sniff('moodle.PHPUnit.TestReturnType');
+        $this->set_fixture(__DIR__ . '/' . $fixture);
+
+        // Define expected results (errors and warnings). Format, array of:
+        // - line => number of problems,  or
+        // - line => array of contents for message / source problem matching.
+        // - line => string of contents for message / source problem matching (only 1).
+        $this->set_errors($errors);
+        $this->set_warnings($warnings);
+
+        // Let's do all the hard work!
+        $this->verify_cs_results();
+    }
+}

moodle/Tests/Sniffs/Namespaces/NamespaceStatementSniffTest.php

@@ -28,7 +28,7 @@
  * @copyright  2023 Andrew Lyons <[email protected]>
  * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  *
- * @covers \MoodleHQ\MoodleCS\moodle\Sniffs\Namespaces\NoLeadingSlashSniff
+ * @covers \MoodleHQ\MoodleCS\moodle\Sniffs\Namespaces\NamespaceStatementSniff
  */
 class NamespaceStatementSniffTest extends MoodleCSBaseTestCase
 {

moodle/Tests/fixtures/moodleutil/istestcaseclass/is_testcase.php

@@ -0,0 +1,5 @@
+<?php
+defined('MOODLE_INTERNAL') || die(); // Make this always the 1st line in all CS fixtures.
+
+class is_testcase extends base_test {
+}

moodle/Tests/fixtures/moodleutil/istestcaseclass/multiple_classes_test.php

@@ -0,0 +1,14 @@
+<?php
+defined('MOODLE_INTERNAL') || die(); // Make this always the 1st line in all CS fixtures.
+
+class is_testcase extends base_test {
+}
+
+class not_a_test_class extends base_test {
+}
+
+class some_other_class_with_test_in_name extends base_test {
+}
+
+class some_other_class_with_test_in_name_not_extending_test {
+}

moodle/Tests/fixtures/moodleutil/istestcaseclass/not_testcase.php

@@ -0,0 +1,5 @@
+<?php
+defined('MOODLE_INTERNAL') || die(); // Make this always the 1st line in all CS fixtures.
+
+class not_a_test_case_class extends base_test {
+}