diff --git a/CHANGELOG.md b/CHANGELOG.md index e578c621..08068986 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,9 @@ This project adheres to [Semantic Versioning](https://semver.org/). The format of this change log follows the advice given at [Keep a CHANGELOG](https://keepachangelog.com). ## [Unreleased] +### Added +- Add new `moodle.PHPUnit.ParentSetUpTearDown` sniff to verify, among other things, that all the `setUp()`, `tearDown()`, `setUpBeforeClass()` and `tearDownAfterClass()` methods in unit tests are properly calling to their parent counterparts. Applies to Moodle 4.5 and up. + ### Changed - Update composer dependencies to current versions, notably `PHP_CodeSniffer` (3.10.1) and `PHPCompatibility` (96072c30). - The `moodle.Commenting.MissingDocblock` sniff will now detect use of the Override attribute (Fixes #155). diff --git a/moodle/Sniffs/PHPUnit/ParentSetUpTearDownSniff.php b/moodle/Sniffs/PHPUnit/ParentSetUpTearDownSniff.php new file mode 100644 index 00000000..b73c271a --- /dev/null +++ b/moodle/Sniffs/PHPUnit/ParentSetUpTearDownSniff.php @@ -0,0 +1,237 @@ +. + +namespace MoodleHQ\MoodleCS\moodle\Sniffs\PHPUnit; + +use MoodleHQ\MoodleCS\moodle\Util\MoodleUtil; +use PHP_CodeSniffer\Files\File; +use PHP_CodeSniffer\Sniffs\Sniff; + +/** + * Checks that a test file setUp and tearDown methods are always calling to parent. + * + * @copyright 2024 onwards Eloy Lafuente (stronk7) {@link https://stronk7.com} + * @license https://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class ParentSetUpTearDownSniff implements Sniff +{ + /** + * @var string[] Methods to verify that they are calling to parent (setup like). + */ + private static array $setUpMethods = [ + 'setUp', + 'setUpBeforeClass', + ]; + + /** + * @var string[] Methods to verify that they are calling to parent (teardown like). + */ + private static array $tearDownMethods = [ + 'tearDown', + 'tearDownAfterClass', + ]; + + /** + * Register for open tag (only process once per file). + */ + public function register(): array { + return [T_OPEN_TAG]; + } + + /** + * Processes php files and perform various checks with file. + * + * @param File $phpcsFile The file being scanned. + * @param int $stackPtr The position in the stack. + */ + public function process(File $phpcsFile, $stackPtr): void { + + // Before starting any check, let's look for various things. + + // If we aren't checking Moodle 4.5dev (405) and up, nothing to check. + // Make and exception for codechecker phpunit tests, so they are run always. + if (!MoodleUtil::meetsMinimumMoodleVersion($phpcsFile, 405) && !MoodleUtil::isUnitTestRunning()) { + return; // @codeCoverageIgnore + } + + // If the file is not a unit test file, nothing to check. + if (!MoodleUtil::isUnitTest($phpcsFile) && !MoodleUtil::isUnitTestRunning()) { + return; // @codeCoverageIgnore + } + + // We only want to do this once per file. + $prevopentag = $phpcsFile->findPrevious(T_OPEN_TAG, $stackPtr - 1); + if ($prevopentag !== false) { + return; // @codeCoverageIgnore + } + + // Get the file tokens, for ease of use. + $tokens = $phpcsFile->getTokens(); + + // These are the methods we are going to check. + $allMethods = array_merge(self::$setUpMethods, self::$tearDownMethods); + + // Iterate over all the classes (hopefully only one, but that's not this sniff problem). + $cStart = $stackPtr; + while ($cStart = $phpcsFile->findNext(T_CLASS, $cStart + 1)) { + // Only interested in classes that are unit test classes. + if (MoodleUtil::isUnitTestCaseClass($phpcsFile, $cStart) === false) { + continue; + } + + // Iterate over all the methods in the class. + $mStart = $cStart; + while ($mStart = $phpcsFile->findNext(T_FUNCTION, $mStart + 1, $tokens[$cStart]['scope_closer'])) { + $method = $phpcsFile->getDeclarationName($mStart); + // Only interested in setUp and tearDown methods. + if (!in_array($method, $allMethods)) { + continue; + } + + // Iterate over all the parent:: calls in the method. + $pStart = $mStart; + $correctParentCalls = []; + while ($pStart = $phpcsFile->findNext(T_PARENT, $pStart + 1, $tokens[$mStart]['scope_closer'])) { + // The next-next token should be the parent method being named. + $methodCall = $phpcsFile->findNext(T_STRING, $pStart + 1, $pStart + 3); + // If we are calling to an incorrect parent method, report it. No fixable. + if ( + $methodCall !== false && + $tokens[$methodCall]['content'] !== $method && + in_array($tokens[$methodCall]['content'], $allMethods) // Other parent calls may be correct. + ) { + $wrongMethod = $tokens[$methodCall]['content']; + // We are calling to incorrect parent method. + $phpcsFile->addError( + 'The %s() method in unit tests cannot call to parent::%s().', + $pStart, + 'Incorrect' . ucfirst($method), + [$method, $wrongMethod] + ); + } + + // If we are calling to the correct parent method, annotate it. + if ( + $methodCall !== false && + $tokens[$methodCall]['content'] === $method + ) { + $correctParentCalls[] = $pStart; + } + } + + // If there are multiple calls to correct parent, report it. Not fixable. + if (count($correctParentCalls) > 1) { + $phpcsFile->addError( + 'The %s() method in unit tests must call to parent::%s() only once.', + end($correctParentCalls), + 'Multiple' . ucfirst($method), + [$method, $method] + ); + } + + // If there are no calls to correct parent, report it. + if (count($correctParentCalls) === 0) { + // Unlikely case of empty method, report it and continue. Not fixable. + // Find the next thing that is not an empty token. + $ignore = \PHP_CodeSniffer\Util\Tokens::$emptyTokens; + + $nextValidStatement = $phpcsFile->findNext( + $ignore, + $tokens[$mStart]['scope_opener'] + 1, + $tokens[$mStart]['scope_closer'], + true + ); + if ($nextValidStatement === false) { + $phpcsFile->addError( + 'The %s() method in unit tests must not be empty', + $mStart, + 'Empty' . ucfirst($method), + [$method] + ); + + continue; + } + + // If the method is not empty, let's report the missing call. Fixable. + $fix = $phpcsFile->addFixableError( + 'The %s() method in unit tests must always call to parent::%s().', + $mStart, + 'Missing' . ucfirst($method), + [$method, $method] + ); + + if ($fix) { + // If the current method is a setUp one, let's add the call at the beginning. + if (in_array($method, self::$setUpMethods)) { + $phpcsFile->fixer->beginChangeset(); + $phpcsFile->fixer->addContent( + $this->findSetUpInsertionPoint($phpcsFile, $mStart), + "\n" . ' parent::' . $method . '();' + ); + $phpcsFile->fixer->endChangeset(); + } + + // If the current method is a tearDown one, let's add the call at the end. + if (in_array($method, self::$tearDownMethods)) { + $phpcsFile->fixer->beginChangeset(); + $phpcsFile->fixer->addContentBefore( + $tokens[$mStart]['scope_closer'] - 1, + ' parent::' . $method . '();' . "\n" + ); + $phpcsFile->fixer->endChangeset(); + } + } + } + } + } + } + + /** + * Find the best insertion point for parent::setUp calls. + * + * While it's technically correct to insert the parent::setUp call at the beginning of the method, + * it's better to insert it after some statements, like global or require/include ones. + * + * @param File $phpcsFile The file being scanned. + * @param int $mStart The position of the method. + * @return int The position where the parent::setUp method should be inserted. + */ + private function findSetUpInsertionPoint(File $phpcsFile, int $mStart): int { + // By default, we are going to insert it at the beginning. + $insertionPoint = $phpcsFile->getTokens()[$mStart]['scope_opener']; + + // Let's find the first token in the method that is not a global, require or include. + // Do it ignoring both whitespace and comments. + $tokens = $phpcsFile->getTokens(); + $mEnd = $tokens[$mStart]['scope_closer']; + + $skipTokens = [T_WHITESPACE, T_COMMENT]; + $allowedTokens = [T_GLOBAL, T_REQUIRE, T_REQUIRE_ONCE, T_INCLUDE, T_INCLUDE_ONCE]; + + while ($findPtr = $phpcsFile->findNext($skipTokens, $insertionPoint + 1, $mEnd, true)) { + // If we find a token that is not allowed, stop looking, insertion point determined. + if (!in_array($tokens[$findPtr]['code'], $allowedTokens)) { + break; + } + + // Arrived here, we can advance the insertion point until the end of the allowed statement. + $insertionPoint = $phpcsFile->findEndOfStatement($findPtr, [T_COMMA]); + } + + return $insertionPoint; + } +} diff --git a/moodle/Tests/Sniffs/PHPUnit/ParentSetUpTearDownSniffTest.php b/moodle/Tests/Sniffs/PHPUnit/ParentSetUpTearDownSniffTest.php new file mode 100644 index 00000000..79ab6670 --- /dev/null +++ b/moodle/Tests/Sniffs/PHPUnit/ParentSetUpTearDownSniffTest.php @@ -0,0 +1,87 @@ +. + +namespace MoodleHQ\MoodleCS\moodle\Tests\Sniffs\PHPUnit; + +use MoodleHQ\MoodleCS\moodle\Tests\MoodleCSBaseTestCase; + +/** + * Test the ParentSetUpTearDownSniff sniff. + * + * @copyright 2024 onwards Eloy Lafuente (stronk7) {@link https://stronk7.com} + * @license https://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * + * @covers \MoodleHQ\MoodleCS\moodle\Sniffs\PHPUnit\ParentSetUpTearDownSniff + */ +class ParentSetUpTearDownSniffTest extends MoodleCSBaseTestCase +{ + /** + * Data provider for self::testParentSetUpTearDown + */ + public static function parentSetUpTearDownProvider(): array { + return [ + 'Correct' => [ + 'fixture' => 'ParentSetUpTearDownCorrect', + 'errors' => [], + 'warnings' => [], + ], + 'Problems' => [ + 'fixture' => 'ParentSetUpTearDownProblems', + 'errors' => [ + 5 => 'The setUp() method in unit tests must not be empty', + 8 => 'The tearDown() method in unit tests must not be empty', + 11 => 'moodle.PHPUnit.ParentSetUpTearDown.EmptySetUpBeforeClass', + 14 => 'moodle.PHPUnit.ParentSetUpTearDown.EmptyTearDownAfterClass', + 21 => 'must call to parent::setUp() only once', + 22 => 'moodle.PHPUnit.ParentSetUpTearDown.IncorrectSetUp', + 27 => 'moodle.PHPUnit.ParentSetUpTearDown.MultipleTearDown', + 29 => 'moodle.PHPUnit.ParentSetUpTearDown.IncorrectTearDown', + 32 => 'moodle.PHPUnit.ParentSetUpTearDown.IncorrectSetUpBeforeClass', + 35 => 'call to parent::setUpBeforeClass() only once', + 40 => 'moodle.PHPUnit.ParentSetUpTearDown.MultipleTearDownAfterClass', + 41 => 'cannot call to parent::setUpBeforeClass()', + 46 => 'moodle.PHPUnit.ParentSetUpTearDown.MissingSetUp', + 62 => 'moodle.PHPUnit.ParentSetUpTearDown.MissingTearDown', + 69 => 'must always call to parent::setUpBeforeClass()', + 83 => 'must always call to parent::tearDownAfterClass()', + 92 => 'moodle.PHPUnit.ParentSetUpTearDown.EmptySetUp', + 93 => 'moodle.PHPUnit.ParentSetUpTearDown.EmptyTearDown', + 94 => 'moodle.PHPUnit.ParentSetUpTearDown.EmptySetUpBeforeClass', + 95 => 'moodle.PHPUnit.ParentSetUpTearDown.EmptyTearDownAfterClass', + ], + 'warnings' => [], + ], + ]; + } + + /** + * @dataProvider parentSetUpTearDownProvider + */ + public function testParentSetUpTearDown( + string $fixture, + array $errors, + array $warnings + ): void { + $this->setStandard('moodle'); + $this->setSniff('moodle.PHPUnit.ParentSetUpTearDown'); + $this->setFixture(sprintf("%s/fixtures/%s.php", __DIR__, $fixture)); + $this->setWarnings($warnings); + $this->setErrors($errors); + + $this->verifyCsResults(); + } +} diff --git a/moodle/Tests/Sniffs/PHPUnit/fixtures/ParentSetUpTearDownCorrect.php b/moodle/Tests/Sniffs/PHPUnit/fixtures/ParentSetUpTearDownCorrect.php new file mode 100644 index 00000000..e1b11727 --- /dev/null +++ b/moodle/Tests/Sniffs/PHPUnit/fixtures/ParentSetUpTearDownCorrect.php @@ -0,0 +1,65 @@ +