From ef0b7645cb8e7a91fbc886045cf0d495e4f02c17 Mon Sep 17 00:00:00 2001 From: "Eloy Lafuente (stronk7)" Date: Mon, 13 May 2024 16:28:02 +0200 Subject: [PATCH 1/7] Better integration tests, with correct exit status checks --- .github/workflows/phpcs.yml | 38 +++++++++++++++---- moodle/Tests/fixtures/integration_test_ci.php | 3 +- 2 files changed, 32 insertions(+), 9 deletions(-) diff --git a/.github/workflows/phpcs.yml b/.github/workflows/phpcs.yml index 277d0467..ce65e813 100644 --- a/.github/workflows/phpcs.yml +++ b/.github/workflows/phpcs.yml @@ -63,18 +63,42 @@ jobs: run: ./vendor/bin/phpunit-coverage-check -t 80 clover.xml - name: Integration tests - if: ${{ (!cancelled()) && (runner.os == 'ubuntu-latest') }} + if: ${{ !cancelled() && matrix.os == 'ubuntu-latest' }} run: | - # There is one failure (exit with error) - vendor/bin/phpcs --standard=moodle moodle/Tests/fixtures/integration_test_ci.php | tee output.txt || [[ $? = 1 ]] + # There is one failure (exit with error 2, because some are fixable). + expectedcode=2 + vendor/bin/phpcs --standard=moodle moodle/Tests/fixtures/integration_test_ci.php | tee output.txt + exitcode="${PIPESTATUS[0]}" + if [[ "${exitcode}" = "${expectedcode}" ]]; then + echo "Ok, got expected ${exitcode} exit code." + else + echo "Error: Expected ${expectedcode}, got ${exitcode} exit code." + exit 1 + fi grep -q "PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY" output.txt - # The failure is fixed (exit with error) - vendor/bin/phpcbf --standard=moodle moodle/Tests/fixtures/integration_test_ci.php | tee output.txt || [[ $? = 1 ]] + # The failure is fixed (exit with error 1, because all fixable ones were fixed). + expectedcode=1 + vendor/bin/phpcbf --standard=moodle moodle/Tests/fixtures/integration_test_ci.php | tee output.txt + exitcode="${PIPESTATUS[0]}" + if [[ "${exitcode}" = "${expectedcode}" ]]; then + echo "Ok, got expected ${exitcode} exit code." + else + echo "Error: Expected ${expectedcode}, got ${exitcode} exit code." + exit 1 + fi grep -q "A TOTAL OF 1 ERROR WERE FIXED IN 1 FILE" output.txt - # So, there isn't any failure any more (exit without error) - vendor/bin/phpcs --standard=moodle moodle/Tests/fixtures/integration_test_ci.php | tee output.txt && [[ $? = 0 ]] + # So, there isn't any failure any more (exit without error, aka, 0) + expectedcode=0 + vendor/bin/phpcs --standard=moodle moodle/Tests/fixtures/integration_test_ci.php | tee output.txt + exitcode="${PIPESTATUS[0]}" + if [[ "${exitcode}" = "${expectedcode}" ]]; then + echo "Ok, got expected ${exitcode} exit code." + else + echo "Error: Expected ${expectedcode}, got ${exitcode} exit code." + exit 1 + fi - name: Mark cancelled jobs as failed if: ${{ cancelled() }} diff --git a/moodle/Tests/fixtures/integration_test_ci.php b/moodle/Tests/fixtures/integration_test_ci.php index fdd4434d..c6d85c9a 100644 --- a/moodle/Tests/fixtures/integration_test_ci.php +++ b/moodle/Tests/fixtures/integration_test_ci.php @@ -1,4 +1,3 @@ - Date: Mon, 20 May 2024 11:54:05 +0200 Subject: [PATCH 2/7] Bump to phpcs 3.10.1 And also to current dev phpcompatibility 96072c30 --- CHANGELOG.md | 2 ++ composer.json | 4 ++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a517c8a1..87e1cef3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,8 @@ 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] +### Changed +- Update composer dependencies to current versions, notably `PHP_CodeSniffer` (3.10.1) and `PHPCompatibility` (96072c30). ## [v3.4.6] - 2024-04-03 ### Fixed diff --git a/composer.json b/composer.json index db93c2d4..d1aad44b 100644 --- a/composer.json +++ b/composer.json @@ -25,9 +25,9 @@ "php": ">=7.4.0", "ext-json": "*", "dealerdirect/phpcodesniffer-composer-installer": "^1.0.0", - "squizlabs/php_codesniffer": "^3.9.0", + "squizlabs/php_codesniffer": "^3.10.1", "phpcsstandards/phpcsextra": "^1.2.1", - "phpcompatibility/php-compatibility": "dev-develop#e5cd2e24" + "phpcompatibility/php-compatibility": "dev-develop#96072c30" }, "config": { "allow-plugins": { From fa1c22b873eee09f751ed557885863a01da9fb18 Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Wed, 17 Apr 2024 10:31:26 +0800 Subject: [PATCH 3/7] Do not warn about missing blocks if a method has the Override attribute Fixes #155 --- CHANGELOG.md | 1 + .../Commenting/MissingDocblockSniff.php | 14 +- .../Commenting/MissingDocblockSniffTest.php | 23 ++- .../with_and_without_overrides.php | 54 +++++ moodle/Tests/Util/AttributesTest.php | 187 ++++++++++++++++++ moodle/Util/Attributes.php | 111 +++++++++++ 6 files changed, 383 insertions(+), 7 deletions(-) create mode 100644 moodle/Tests/Sniffs/Commenting/fixtures/MissingDocblock/with_and_without_overrides.php create mode 100644 moodle/Tests/Util/AttributesTest.php create mode 100644 moodle/Util/Attributes.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 87e1cef3..e578c621 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ The format of this change log follows the advice given at [Keep a CHANGELOG](htt ## [Unreleased] ### 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). ## [v3.4.6] - 2024-04-03 ### Fixed diff --git a/moodle/Sniffs/Commenting/MissingDocblockSniff.php b/moodle/Sniffs/Commenting/MissingDocblockSniff.php index 4467a067..8999a59a 100644 --- a/moodle/Sniffs/Commenting/MissingDocblockSniff.php +++ b/moodle/Sniffs/Commenting/MissingDocblockSniff.php @@ -17,6 +17,7 @@ namespace MoodleHQ\MoodleCS\moodle\Sniffs\Commenting; +use MoodleHQ\MoodleCS\moodle\Util\Attributes; use MoodleHQ\MoodleCS\moodle\Util\Docblocks; use MoodleHQ\MoodleCS\moodle\Util\MoodleUtil; use MoodleHQ\MoodleCS\moodle\Util\TokenUtil; @@ -183,6 +184,17 @@ protected function processFunctions(File $phpcsFile, int $stackPtr): void { foreach ($missingDocblocks as $typePtr => $extendsOrImplements) { $token = $tokens[$typePtr]; + if ($extendsOrImplements) { + $attributes = Attributes::getAttributePointers($phpcsFile, $typePtr); + foreach ($attributes as $attributePtr) { + $attribute = Attributes::getAttributeProperties($phpcsFile, $attributePtr); + if ($attribute['attribute_name'] === '\Override') { + // Skip methods that are marked as overrides. + continue 2; + } + } + } + $objectName = TokenUtil::getObjectName($phpcsFile, $typePtr); $objectType = TokenUtil::getObjectType($phpcsFile, $typePtr); @@ -195,8 +207,6 @@ protected function processFunctions(File $phpcsFile, int $stackPtr): void { [$objectType, $objectName] ); } - } elseif ($extendsOrImplements) { - $phpcsFile->addWarning('Missing docblock for %s %s', $typePtr, 'Missing', [$objectType, $objectName]); } else { $phpcsFile->addError('Missing docblock for %s %s', $typePtr, 'Missing', [$objectType, $objectName]); } diff --git a/moodle/Tests/Sniffs/Commenting/MissingDocblockSniffTest.php b/moodle/Tests/Sniffs/Commenting/MissingDocblockSniffTest.php index 39f5d67b..25da9538 100644 --- a/moodle/Tests/Sniffs/Commenting/MissingDocblockSniffTest.php +++ b/moodle/Tests/Sniffs/Commenting/MissingDocblockSniffTest.php @@ -18,9 +18,6 @@ namespace MoodleHQ\MoodleCS\moodle\Tests\Sniffs\Commenting; use MoodleHQ\MoodleCS\moodle\Tests\MoodleCSBaseTestCase; -use PHP_CodeSniffer\Config; -use PHP_CodeSniffer\Files\DummyFile; -use PHP_CodeSniffer\Ruleset; /** * Test the MissingDocblockSniff sniff. @@ -68,11 +65,11 @@ public static function docblockCorrectnessProvider(): array { 159 => 'Missing docblock for function test_method', 166 => 'Missing docblock for function test_method', 170 => 'Missing docblock for class example_extends', + 171 => 'Missing docblock for function test_method', 175 => 'Missing docblock for class example_implements', + 176 => 'Missing docblock for function test_method', ], 'warnings' => [ - 171 => 'Missing docblock for function test_method', - 176 => 'Missing docblock for function test_method', ], ], 'File level tag, no class' => [ @@ -166,6 +163,22 @@ public static function docblockCorrectnessProvider(): array { 18 => 'Missing docblock for function this_is_a_dataprovider in testcase', ], ], + 'With and without Overrides attributes' => [ + 'fixture' => 'with_and_without_overrides', + 'fixtureFilename' => null, + 'errors' => [ + 1 => 'Missing docblock for file with_and_without_overrides.php', + 11 => 'Missing docblock for function has_override', + 13 => 'Missing docblock for function no_override', + 21 => 'Missing docblock for function has_override', + 23 => 'Missing docblock for function no_override', + 33 => 'Missing docblock for function no_override', + 43 => 'Missing docblock for function no_override', + 53 => 'Missing docblock for function no_override', + ], + 'warnings' => [ + ], + ], ]; if (version_compare(PHP_VERSION, '8.0.0') >= 0) { diff --git a/moodle/Tests/Sniffs/Commenting/fixtures/MissingDocblock/with_and_without_overrides.php b/moodle/Tests/Sniffs/Commenting/fixtures/MissingDocblock/with_and_without_overrides.php new file mode 100644 index 00000000..fbc57766 --- /dev/null +++ b/moodle/Tests/Sniffs/Commenting/fixtures/MissingDocblock/with_and_without_overrides.php @@ -0,0 +1,54 @@ +. + +namespace MoodleHQ\MoodleCS\moodle\Tests\Util; + +use MoodleHQ\MoodleCS\moodle\Tests\MoodleCSBaseTestCase; +use MoodleHQ\MoodleCS\moodle\Util\Attributes; +use PHP_CodeSniffer\Config; +use PHP_CodeSniffer\Files\DummyFile; +use PHP_CodeSniffer\Ruleset; + +/** + * Test the Attributes specific moodle utilities class + * + * @copyright 2024 onwards Andrew Lyons + * @license https://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * + * @covers \MoodleHQ\MoodleCS\moodle\Util\Attributes + */ +class AttributesTest extends MoodleCSBaseTestCase +{ + /** + * @dataProvider validTagsProvider + */ + public function testGetAttributePointers( + string $content, + $stackPtrSearch, + ?array $expectations + ): void { + $config = new Config([]); + $ruleset = new Ruleset($config); + + $phpcsFile = new DummyFile($content, $ruleset, $config); + $phpcsFile->process(); + + $searchPtr = $phpcsFile->findNext($stackPtrSearch, 0); + + $pointers = Attributes::getAttributePointers($phpcsFile, $searchPtr); + if (count($expectations)) { + foreach ($expectations as $expectation) { + $this->assertCount( + $expectation['count'], + array_filter($pointers, function ($pointer) use ($expectation, $phpcsFile) { + $properties = Attributes::getAttributeProperties($phpcsFile, $pointer); + + return $properties['attribute_name'] === $expectation['name']; + }) + ); + } + } else { + $this->assertEmpty($pointers); + } + } + + public static function validTagsProvider(): array { + return [ + 'No attributes' => [ + ' [ + ' '\\Override', 'count' => 1], + ], + ], + 'Multiple Attribute matches (same)' => [ + ' '\\Override', 'count' => 2], + ], + ], + 'Multiple Attribute matches (different)' => [ + ' '\\Override', 'count' => 2], + ['name' => '\\Other', 'count' => 1], + ], + ], + 'Attribute on other funciton' => [ + ' [ + ' '\\Override', 'count' => 1], + ['name' => '\\Route', 'count' => 2], + ['name' => '\\core\\attribute\\deprecated', 'count' => 1], + ], + ], + 'Attribute start will only get that attribute' => [ + ' '\\Route', 'count' => 1], + ], + ], + 'Attribute End will only get that attribute' => [ + ' '\\Route', 'count' => 1], + ], + ], + ]; + } + + public function testGetAttributePropertiesNotAnAttribute(): void { + $config = new Config([]); + $ruleset = new Ruleset($config); + + $content = <<process(); + + $searchPtr = $phpcsFile->findNext(T_FUNCTION, 0); + + $this->assertNull(Attributes::getAttributeProperties($phpcsFile, $searchPtr)); + } +} diff --git a/moodle/Util/Attributes.php b/moodle/Util/Attributes.php new file mode 100644 index 00000000..ef6ad71d --- /dev/null +++ b/moodle/Util/Attributes.php @@ -0,0 +1,111 @@ +. + +namespace MoodleHQ\MoodleCS\moodle\Util; + +use PHP_CodeSniffer\Files\File; +use PHPCSUtils\Utils\Context; +use PHPCSUtils\Utils\Namespaces; + +/** + * Utilities related to PHP Attributes. + * + * @copyright 2024 Andrew Lyons + * @license https://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +abstract class Attributes +{ + /** + * Get the pointer for an Attribute on an Attributable object. + * + * @param File $phpcsFile + * @param int $stackPtr + * @return array + */ + public static function getAttributePointers( + File $phpcsFile, + int $stackPtr + ): array { + $tokens = $phpcsFile->getTokens(); + $attributes = []; + + $stopAt = [ + T_DOC_COMMENT_CLOSE_TAG, + T_CLOSE_CURLY_BRACKET, + T_OPEN_CURLY_BRACKET, + T_SEMICOLON, + ]; + + for ($attributePtr = $stackPtr; $attributePtr > 0; $attributePtr--) { + $token = $tokens[$attributePtr]; + // The phpcs parser places an attribute_opener and attribute_closer on every part of an attribute. + if (isset($token['attribute_opener'])) { + $attributePtr = $token['attribute_opener']; + $attributes[] = $attributePtr; + } + + if (in_array($token['code'], $stopAt)) { + break; + } + } + + return $attributes; + } + + /** + * Get the properties of an Attribute. + * + * Note: The attribute name is not currently qualified relative to the current namespace or any imported classes. + * + * @param File $phpcsFile + * @param int $stackPtr + * @return array|null + */ + public static function getAttributeProperties( + File $phpcsFile, + int $stackPtr + ): ?array { + $tokens = $phpcsFile->getTokens(); + if (!isset($tokens[$stackPtr]['attribute_opener'])) { + return null; + } + + $opener = $tokens[$stackPtr]['attribute_opener']; + $closer = $tokens[$stackPtr]['attribute_closer']; + + $properties = [ + 'attribute_opener' => $opener, + 'attribute_closer' => $closer, + 'attribute_name' => null, + ]; + + $stopAt = [ + T_OPEN_PARENTHESIS, + ]; + + for ($i = $opener + 1; $i < $closer; $i++) { + if (in_array($tokens[$i]['code'], $stopAt)) { + break; + } + $properties['attribute_name'] .= $tokens[$i]['content']; + } + + // TODO Get the qualified name. + + return $properties; + } +} From 6a458cb2c129c65cc0e6a2f92833ac05762ae44a Mon Sep 17 00:00:00 2001 From: "Eloy Lafuente (stronk7)" Date: Tue, 28 May 2024 08:44:41 +0200 Subject: [PATCH 4/7] New sniff to detect correct setUp/tearDown parent calls It detects various situations like: - Missing calls to parent. - Dupe calls. - Incorrect calls. And, when possible, tries to fix the missing ones (not others) by adding them in a correct place: - For setUp cases, at the beginning of the method. - For tearDown cases, at the end of the method. Of course, covered with tests. Fixes #106 --- CHANGELOG.md | 3 + .../PHPUnit/ParentSetUpTearDownSniff.php | 222 ++++++++++++++++++ .../PHPUnit/ParentSetUpTearDownSniffTest.php | 83 +++++++ .../fixtures/ParentSetUpTearDownCorrect.php | 69 ++++++ .../fixtures/ParentSetUpTearDownProblems.php | 89 +++++++ .../ParentSetUpTearDownProblems.php.fixed | 97 ++++++++ 6 files changed, 563 insertions(+) create mode 100644 moodle/Sniffs/PHPUnit/ParentSetUpTearDownSniff.php create mode 100644 moodle/Tests/Sniffs/PHPUnit/ParentSetUpTearDownSniffTest.php create mode 100644 moodle/Tests/Sniffs/PHPUnit/fixtures/ParentSetUpTearDownCorrect.php create mode 100644 moodle/Tests/Sniffs/PHPUnit/fixtures/ParentSetUpTearDownProblems.php create mode 100644 moodle/Tests/Sniffs/PHPUnit/fixtures/ParentSetUpTearDownProblems.php.fixed 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..d9b9e2d5 --- /dev/null +++ b/moodle/Sniffs/PHPUnit/ParentSetUpTearDownSniff.php @@ -0,0 +1,222 @@ +. + +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. Fixable. + if (count($correctParentCalls) === 0) { + // Any weird case where the method is empty, we need at very least 1 line. Skip. + $startLine = $tokens[$tokens[$mStart]['scope_opener']]['line']; + $endLine = $tokens[$tokens[$mStart]['scope_closer']]['line']; + if ($startLine === $endLine) { + continue; + } + + $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..7076df7b --- /dev/null +++ b/moodle/Tests/Sniffs/PHPUnit/ParentSetUpTearDownSniffTest.php @@ -0,0 +1,83 @@ +. + +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 => 'must always call to parent::setUp()', + 8 => 'tearDown() method in unit tests must always call', + 11 => 'moodle.PHPUnit.ParentSetUpTearDown.MissingSetUpBeforeClass', + 14 => 'moodle.PHPUnit.ParentSetUpTearDown.MissingTearDownAfterClass', + 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()', + ], + '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..5ba77654 --- /dev/null +++ b/moodle/Tests/Sniffs/PHPUnit/fixtures/ParentSetUpTearDownCorrect.php @@ -0,0 +1,69 @@ + Date: Fri, 31 May 2024 15:30:51 +0800 Subject: [PATCH 5/7] Add Error for empty setUp/tearDown methods --- .../PHPUnit/ParentSetUpTearDownSniff.php | 25 +++++++++++++++---- .../PHPUnit/ParentSetUpTearDownSniffTest.php | 12 ++++++--- .../fixtures/ParentSetUpTearDownCorrect.php | 4 --- .../fixtures/ParentSetUpTearDownProblems.php | 15 ++++++++--- .../ParentSetUpTearDownProblems.php.fixed | 19 ++++++++------ 5 files changed, 50 insertions(+), 25 deletions(-) diff --git a/moodle/Sniffs/PHPUnit/ParentSetUpTearDownSniff.php b/moodle/Sniffs/PHPUnit/ParentSetUpTearDownSniff.php index d9b9e2d5..b73c271a 100644 --- a/moodle/Sniffs/PHPUnit/ParentSetUpTearDownSniff.php +++ b/moodle/Sniffs/PHPUnit/ParentSetUpTearDownSniff.php @@ -143,15 +143,30 @@ public function process(File $phpcsFile, $stackPtr): void { ); } - // If there are no calls to correct parent, report it. Fixable. + // If there are no calls to correct parent, report it. if (count($correctParentCalls) === 0) { - // Any weird case where the method is empty, we need at very least 1 line. Skip. - $startLine = $tokens[$tokens[$mStart]['scope_opener']]['line']; - $endLine = $tokens[$tokens[$mStart]['scope_closer']]['line']; - if ($startLine === $endLine) { + // 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, diff --git a/moodle/Tests/Sniffs/PHPUnit/ParentSetUpTearDownSniffTest.php b/moodle/Tests/Sniffs/PHPUnit/ParentSetUpTearDownSniffTest.php index 7076df7b..79ab6670 100644 --- a/moodle/Tests/Sniffs/PHPUnit/ParentSetUpTearDownSniffTest.php +++ b/moodle/Tests/Sniffs/PHPUnit/ParentSetUpTearDownSniffTest.php @@ -42,10 +42,10 @@ public static function parentSetUpTearDownProvider(): array { 'Problems' => [ 'fixture' => 'ParentSetUpTearDownProblems', 'errors' => [ - 5 => 'must always call to parent::setUp()', - 8 => 'tearDown() method in unit tests must always call', - 11 => 'moodle.PHPUnit.ParentSetUpTearDown.MissingSetUpBeforeClass', - 14 => 'moodle.PHPUnit.ParentSetUpTearDown.MissingTearDownAfterClass', + 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', @@ -58,6 +58,10 @@ public static function parentSetUpTearDownProvider(): array { 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' => [], ], diff --git a/moodle/Tests/Sniffs/PHPUnit/fixtures/ParentSetUpTearDownCorrect.php b/moodle/Tests/Sniffs/PHPUnit/fixtures/ParentSetUpTearDownCorrect.php index 5ba77654..e1b11727 100644 --- a/moodle/Tests/Sniffs/PHPUnit/fixtures/ParentSetUpTearDownCorrect.php +++ b/moodle/Tests/Sniffs/PHPUnit/fixtures/ParentSetUpTearDownCorrect.php @@ -24,10 +24,6 @@ public static function tearDownAfterClass(): void { } class another_correct_setup_teardown_test extends Something { - public function setUp(): void {} - public function tearDown(): void {} - public static function setUpBeforeClass(): void {} - public static function tearDownAfterClass(): void { } // Same line. public function ignoredMethod(): void { parent::setUp(); parent::tearDown(); diff --git a/moodle/Tests/Sniffs/PHPUnit/fixtures/ParentSetUpTearDownProblems.php b/moodle/Tests/Sniffs/PHPUnit/fixtures/ParentSetUpTearDownProblems.php index c4a5dc6a..35a95c6f 100644 --- a/moodle/Tests/Sniffs/PHPUnit/fixtures/ParentSetUpTearDownProblems.php +++ b/moodle/Tests/Sniffs/PHPUnit/fixtures/ParentSetUpTearDownProblems.php @@ -1,16 +1,16 @@ Date: Wed, 17 Apr 2024 09:29:34 +0800 Subject: [PATCH 6/7] Add new sniff to detect and remove constructor @return docs (#157) --- CHANGELOG.md | 1 + .../Commenting/ConstructorReturnSniff.php | 120 ++++++++++++++++++ .../Commenting/ConstructorReturnSniffTest.php | 64 ++++++++++ .../fixtures/ConstructorReturn/general.php | 76 +++++++++++ .../ConstructorReturn/general.php.fixed | 74 +++++++++++ 5 files changed, 335 insertions(+) create mode 100644 moodle/Sniffs/Commenting/ConstructorReturnSniff.php create mode 100644 moodle/Tests/Sniffs/Commenting/ConstructorReturnSniffTest.php create mode 100644 moodle/Tests/Sniffs/Commenting/fixtures/ConstructorReturn/general.php create mode 100644 moodle/Tests/Sniffs/Commenting/fixtures/ConstructorReturn/general.php.fixed diff --git a/CHANGELOG.md b/CHANGELOG.md index 08068986..8003d0a0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ The format of this change log follows the advice given at [Keep a CHANGELOG](htt ## [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. +- Add new `moodle.Commenting.ConstructorReturn` sniff to check that constructors do not document a return value. ### Changed - Update composer dependencies to current versions, notably `PHP_CodeSniffer` (3.10.1) and `PHPCompatibility` (96072c30). diff --git a/moodle/Sniffs/Commenting/ConstructorReturnSniff.php b/moodle/Sniffs/Commenting/ConstructorReturnSniff.php new file mode 100644 index 00000000..594cc7b9 --- /dev/null +++ b/moodle/Sniffs/Commenting/ConstructorReturnSniff.php @@ -0,0 +1,120 @@ +. + +namespace MoodleHQ\MoodleCS\moodle\Sniffs\Commenting; + +use MoodleHQ\MoodleCS\moodle\Util\Docblocks; +use MoodleHQ\MoodleCS\moodle\Util\TokenUtil; +use PHP_CodeSniffer\Files\File; +use PHP_CodeSniffer\Sniffs\Sniff; + +/** + * Checks that all files an classes have appropriate docs. + * + * @copyright 2024 Andrew Lyons + * @license https://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class ConstructorReturnSniff implements Sniff +{ + /** + * Register for class tags. + */ + public function register() { + + return [ + T_CLASS, + ]; + } + + /** + * 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) { + $tokens = $phpcsFile->getTokens(); + $endClassPtr = $tokens[$stackPtr]['scope_closer']; + + while ( + ($methodPtr = $phpcsFile->findNext(T_FUNCTION, $stackPtr + 1, $endClassPtr)) !== false + ) { + $this->processClassMethod($phpcsFile, $methodPtr); + $stackPtr = $methodPtr; + } + } + + /** + * Processes the class method. + * + * @param File $phpcsFile The file being scanned. + * @param int $stackPtr The position in the stack. + */ + protected function processClassMethod(File $phpcsFile, int $stackPtr): void { + $objectName = TokenUtil::getObjectName($phpcsFile, $stackPtr); + if ($objectName !== '__constructor') { + // We only care about constructors. + return; + } + + // Get docblock. + $docblockPtr = Docblocks::getDocBlockPointer($phpcsFile, $stackPtr); + if ($docblockPtr === null) { + // No docblocks for this constructor. + return; + } + + $returnTokens = Docblocks::getMatchingDocTags($phpcsFile, $docblockPtr, '@return'); + if (count($returnTokens) === 0) { + // No @return tag in the docblock. + return; + } + + $fix = $phpcsFile->addFixableError( + 'Constructor should not have a return tag in the docblock', + $returnTokens[0], + 'ConstructorReturn' + ); + if ($fix) { + $tokens = $phpcsFile->getTokens(); + $phpcsFile->fixer->beginChangeset(); + + // Find the tokens at the start and end of the line. + $lineStart = $phpcsFile->findFirstOnLine(T_DOC_COMMENT_STAR, $returnTokens[0]); + if ($lineStart === false) { + $lineStart = $returnTokens[0]; + } + + $ptr = $phpcsFile->findNext(T_DOC_COMMENT_WHITESPACE, $lineStart); + for ($lineEnd = $lineStart; $lineEnd < $tokens[$docblockPtr]['comment_closer']; $lineEnd++) { + if ($tokens[$lineEnd]['line'] !== $tokens[$lineStart]['line']) { + break; + } + } + + if ($tokens[$lineEnd]['code'] === T_DOC_COMMENT_CLOSE_TAG) { + $lineEnd--; + } + + for ($ptr = $lineStart; $ptr <= $lineEnd; $ptr++) { + $phpcsFile->fixer->replaceToken($ptr, ''); + } + + $phpcsFile->fixer->endChangeset(); + } + } +} diff --git a/moodle/Tests/Sniffs/Commenting/ConstructorReturnSniffTest.php b/moodle/Tests/Sniffs/Commenting/ConstructorReturnSniffTest.php new file mode 100644 index 00000000..c27b72a9 --- /dev/null +++ b/moodle/Tests/Sniffs/Commenting/ConstructorReturnSniffTest.php @@ -0,0 +1,64 @@ +. + +namespace MoodleHQ\MoodleCS\moodle\Tests\Sniffs\Commenting; + +use MoodleHQ\MoodleCS\moodle\Tests\MoodleCSBaseTestCase; + +/** + * Test the ConstructorReturnSniff sniff. + * + * @copyright 2024 onwards Andrew Lyons + * @license https://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * + * @covers \MoodleHQ\MoodleCS\moodle\Sniffs\Commenting\ConstructorReturnSniff + */ +class ConstructorReturnSniffTest extends MoodleCSBaseTestCase +{ + /** + * @dataProvider docblockCorrectnessProvider + */ + public function testMissingDocblockSniff( + string $fixture, + array $errors, + array $warnings + ): void { + $this->setStandard('moodle'); + $this->setSniff('moodle.Commenting.ConstructorReturn'); + $this->setFixture(sprintf("%s/fixtures/ConstructorReturn/%s.php", __DIR__, $fixture)); + $this->setWarnings($warnings); + $this->setErrors($errors); + + $this->verifyCsResults(); + } + + public static function docblockCorrectnessProvider(): array { + $cases = [ + [ + 'fixture' => 'general', + 'errors' => [ + 43 => 'Constructor should not have a return tag in the docblock', + 52 => 'Constructor should not have a return tag in the docblock', + 60 => 'Constructor should not have a return tag in the docblock', + 71 => 'Constructor should not have a return tag in the docblock', + ], + 'warnings' => [], + ], + ]; + return $cases; + } +} diff --git a/moodle/Tests/Sniffs/Commenting/fixtures/ConstructorReturn/general.php b/moodle/Tests/Sniffs/Commenting/fixtures/ConstructorReturn/general.php new file mode 100644 index 00000000..2207678d --- /dev/null +++ b/moodle/Tests/Sniffs/Commenting/fixtures/ConstructorReturn/general.php @@ -0,0 +1,76 @@ + Date: Fri, 31 May 2024 18:28:39 +0200 Subject: [PATCH 7/7] Prepare for v3.4.7 release --- CHANGELOG.md | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8003d0a0..148fb981 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,8 @@ 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] + +## [v3.4.7] - 2024-05-31 ### 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. - Add new `moodle.Commenting.ConstructorReturn` sniff to check that constructors do not document a return value. @@ -14,6 +16,9 @@ The format of this change log follows the advice given at [Keep a CHANGELOG](htt - 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). +### Fixed +- Various fixes to own (GH workflow) integration tests. + ## [v3.4.6] - 2024-04-03 ### Fixed - Solved a problem where Windows file paths were not normalised leading to false positive results on some path-based sniffs. @@ -217,7 +222,8 @@ All features are maintained and no new features have been introduced to either t All the details about [previous releases] can be found in [local_codechecker](https://github.com/moodlehq/moodle-local_codechecker) own change log. -[Unreleased]: https://github.com/moodlehq/moodle-cs/compare/v3.4.6...main +[Unreleased]: https://github.com/moodlehq/moodle-cs/compare/v3.4.7...main +[v3.4.7]: https://github.com/moodlehq/moodle-cs/compare/v3.4.6...v3.4.7 [v3.4.6]: https://github.com/moodlehq/moodle-cs/compare/v3.4.5...v3.4.6 [v3.4.5]: https://github.com/moodlehq/moodle-cs/compare/v3.4.4...v3.4.5 [v3.4.4]: https://github.com/moodlehq/moodle-cs/compare/v3.4.3...v3.4.4