From f310cd2205e8e5f4558541b248ca0446d8897c2e Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Fri, 15 Sep 2023 23:21:13 +0800 Subject: [PATCH] Start writing tests for unit test checks This change includes: - detection of private providers - detection of providers which do not exist - detection of incorrect casing of the dataProvider declaration - detection of providers which do not have a correct return type - detection of providers which are not static - detection of providers whose names start with test_ Fixes #42 --- moodle-extra/ruleset.xml | 15 + .../Sniffs/PHPUnit/TestCaseProviderSniff.php | 368 ++++++++++++++++++ moodle/Tests/MoodleCSBaseTestCase.php | 37 +- moodle/Tests/MoodleUtilTest.php | 198 ++++++++++ moodle/Tests/PHPUnitTestCaseProviderTest.php | 152 ++++++++ .../moodleutil/test_with_methods_to_find.php | 23 ++ .../provider/complex_provider_test.php | 43 ++ .../provider/complex_provider_test.php.fixed | 43 ++ .../phpunit/provider/correct_test.php | 22 ++ .../phpunit/provider/provider_casing_test.php | 15 + .../provider/provider_casing_test.php.fixed | 15 + .../provider/provider_not_found_test.php | 18 + .../phpunit/provider/provider_prefix_test.php | 15 + .../provider/provider_returntype_test.php | 53 +++ .../provider/provider_visibility_test.php | 37 ++ .../provider_visibility_test.php.fixed | 37 ++ .../provider/static_providers_fix_test.php | 45 +++ .../static_providers_fix_test.php.fixed | 45 +++ .../provider/static_providers_test.php | 44 +++ .../provider/static_providers_test.php.fixed | 44 +++ moodle/Util/MoodleUtil.php | 104 +++++ moodle/ruleset.xml | 10 + 22 files changed, 1349 insertions(+), 34 deletions(-) create mode 100644 moodle/Sniffs/PHPUnit/TestCaseProviderSniff.php create mode 100644 moodle/Tests/PHPUnitTestCaseProviderTest.php create mode 100644 moodle/Tests/fixtures/moodleutil/test_with_methods_to_find.php create mode 100644 moodle/Tests/fixtures/phpunit/provider/complex_provider_test.php create mode 100644 moodle/Tests/fixtures/phpunit/provider/complex_provider_test.php.fixed create mode 100644 moodle/Tests/fixtures/phpunit/provider/correct_test.php create mode 100644 moodle/Tests/fixtures/phpunit/provider/provider_casing_test.php create mode 100644 moodle/Tests/fixtures/phpunit/provider/provider_casing_test.php.fixed create mode 100644 moodle/Tests/fixtures/phpunit/provider/provider_not_found_test.php create mode 100644 moodle/Tests/fixtures/phpunit/provider/provider_prefix_test.php create mode 100644 moodle/Tests/fixtures/phpunit/provider/provider_returntype_test.php create mode 100644 moodle/Tests/fixtures/phpunit/provider/provider_visibility_test.php create mode 100644 moodle/Tests/fixtures/phpunit/provider/provider_visibility_test.php.fixed create mode 100644 moodle/Tests/fixtures/phpunit/provider/static_providers_fix_test.php create mode 100644 moodle/Tests/fixtures/phpunit/provider/static_providers_fix_test.php.fixed create mode 100644 moodle/Tests/fixtures/phpunit/provider/static_providers_test.php create mode 100644 moodle/Tests/fixtures/phpunit/provider/static_providers_test.php.fixed diff --git a/moodle-extra/ruleset.xml b/moodle-extra/ruleset.xml index b392e02..462416a 100644 --- a/moodle-extra/ruleset.xml +++ b/moodle-extra/ruleset.xml @@ -84,4 +84,19 @@ --> + + + + + + + diff --git a/moodle/Sniffs/PHPUnit/TestCaseProviderSniff.php b/moodle/Sniffs/PHPUnit/TestCaseProviderSniff.php new file mode 100644 index 0000000..1aa3b27 --- /dev/null +++ b/moodle/Sniffs/PHPUnit/TestCaseProviderSniff.php @@ -0,0 +1,368 @@ +. + +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 PHP_CodeSniffer\Util\Tokens; +use PHPCSUtils\Utils\FunctionDeclarations; + +/** + * 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 TestCaseProviderSniff implements Sniff { + + /** + * Whether to autofix static providers. + * + * @var bool + */ + public $autofixStaticProviders = false; + + /** + * 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 $file The file being scanned. + * @param int $pointer The position in the stack. + */ + public function process(File $file, $pointer): void + { + // 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(); + + // In various places we are going to ignore class/method prefixes (private, abstract...) + // and whitespace, create an array for all them. + $skipTokens = Tokens::$methodPrefixes + [T_WHITESPACE => T_WHITESPACE]; + + // 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)) { + $class = $file->getDeclarationName($cStart); + + // Only if the class is extending something. + // TODO: We could add a list of valid classes once we have a class-map available. + if (!$file->findNext(T_EXTENDS, $cStart + 1, $tokens[$cStart]['scope_opener'])) { + continue; + } + + // Ignore any classname which does not end in "_test". + if (substr($class, -5) !== '_test') { + continue; + } + + // 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; + } + + // Let's see if the method has any phpdoc block (first non skip token must be end of phpdoc comment). + $docPointer = $file->findPrevious($skipTokens, $mStart - 1, null, true); + + // Found a phpdoc block, let's look for @dataProvider tag. + if ($tokens[$docPointer]['code'] === T_DOC_COMMENT_CLOSE_TAG) { + $docStart = $tokens[$docPointer]['comment_opener']; + while ($docPointer) { // Let's look upwards, until the beginning of the phpdoc block. + $docPointer = $file->findPrevious(T_DOC_COMMENT_TAG, $docPointer - 1, $docStart); + if ($docPointer) { + $docTag = trim($tokens[$docPointer]['content']); + $docTagLC = strtolower($docTag); + switch ($docTagLC) { + case '@dataprovider': + // Validate basic syntax (FQCN or ::). + $this->checkDataProvider($file, $docPointer); + break; + } + } + } + } + + // Advance until the end of the method, if possible, to find the next one quicker. + $mStart = $tokens[$mStart]['scope_closer'] ?? $pointer + 1; + } + } + } + + /** + * Perform a basic syntax cheking of the values of the @dataProvider tag. + * + * @param File $file The file being scanned + * @param int $pointer pointer to the token that contains the tag. Calculations are based on that. + * @return void + */ + protected function checkDataProvider( + File $file, + int $pointer + ) { + // Get the file tokens, for ease of use. + $tokens = $file->getTokens(); + $tag = $tokens[$pointer]['content']; + $methodName = $tokens[$pointer + 2]['content']; + $testPointer = $file->findNext(T_FUNCTION, $pointer + 2); + $testName = FunctionDeclarations::getName($file, $testPointer); + + if ($tag !== '@dataProvider') { + $fix = $file->addFixableError( + 'Wrong @dataProvider tag: %s provided, @dataProvider expected', + $pointer, + 'dataProviderNaming', + [$tag] + ); + + if ($fix) { + $file->fixer->beginChangeset(); + $file->fixer->replaceToken($pointer, '@dataProvider'); + $file->fixer->endChangeset(); + } + } + + if ($tokens[$pointer + 2]['code'] !== T_DOC_COMMENT_STRING) { + $file->addError( + 'Wrong @dataProvider tag specified for test %s, it must be followed by a space and a method name.', + $pointer + 2, + 'dataProviderSyntaxMethodnameMissing', + [ + $testName, + ] + ); + + // The remaining checks all relate to the method name, so we can't continue. + return; + } + + // Check that the method name is valid. + // It must _not_ start with `test_`. + if (substr($methodName, 0, 5) === 'test_') { + $file->addError( + 'Data provider must not start with "test_". "%s" provided.', + $pointer + 2, + 'dataProviderSyntaxMethodnameInvalid', + [ + $methodName, + ] + ); + } + + // Find the method itself. + $classPointer = $file->findPrevious(T_CLASS, $pointer - 1); + $providerPointer = MoodleUtil::findClassMethodPointer($file, $classPointer, $methodName); + if ($providerPointer === null) { + $file->addError( + 'Data provider method "%s" not found.', + $pointer + 2, + 'dataProviderSyntaxMethodNotFound', + [ + $methodName, + ] + ); + + return; + } + + // https://docs.phpunit.de/en/9.6/writing-tests-for-phpunit.html#data-providers + // A data provider method must be public and either return an array of arrays + // or an object that implements the Iterator interface and yields an array for + // each iteration step. For each array that is part of the collection the test + // method will be called with the contents of the array as its arguments. + + // Check that the method is public. + $methodProps = $file->getMethodProperties($providerPointer); + if (!$methodProps['scope_specified']) { + $fix = $file->addFixableError( + 'Data provider method "%s" visibility should be specified.', + $providerPointer, + 'dataProviderSyntaxMethodVisibilityNotSpecified', + [ + $methodName, + ] + ); + + if ($fix) { + $file->fixer->beginChangeset(); + if ($methodProps['is_static']) { + $staticPointer = $file->findPrevious(T_STATIC, $providerPointer - 1); + $file->fixer->addContentBefore($staticPointer, 'public '); + } else { + $file->fixer->addContentBefore($providerPointer, 'public '); + } + $file->fixer->endChangeset(); + } + } else if ($methodProps['scope'] !== 'public') { + $scopePointer = $file->findPrevious(Tokens::$scopeModifiers, $providerPointer - 1); + $fix = $file->addFixableError( + 'Data provider method "%s" must be public.', + $scopePointer, + 'dataProviderSyntaxMethodNotPublic', + [ + $methodName, + ] + ); + + if ($fix) { + $file->fixer->beginChangeset(); + $file->fixer->replaceToken($scopePointer, 'public'); + $file->fixer->endChangeset(); + } + } + + // Check the return type. + switch ($methodProps['return_type']) { + case 'array': + case 'Generator': + case 'Iterable': + // All valid + break; + default: + $file->addError( + 'Data provider method "%s" must return an array, a Generator or an Iterable.', + $pointer + 2, + 'dataProviderSyntaxMethodInvalidReturnType', + [ + $methodName, + ] + ); + } + + // In preparation for PHPUnit 10, we want to recommend that data providers are statically defined. + if (!$methodProps['is_static']) { + $supportAutomatedFix = true; + if (!$this->autofixStaticProviders) { + $supportAutomatedFix = false; + } else { + // We can make this fixable if the method does not contain any `$this`. + // Search the body. + $bodyStart = $tokens[$providerPointer]['scope_opener'] + 1; + $bodyEnd = $tokens[$providerPointer]['scope_closer'] - 1; + while ($token = $file->findNext(T_VARIABLE, $bodyStart, $bodyEnd)) { + if ($tokens[$token]['content'] === '$this') { + $supportAutomatedFix = false; + break; + } + } + } + + if (!$supportAutomatedFix) { + $file->addWarning( + 'Data provider method "%s" will need to be converted to static in future.', + $pointer + 2, + 'dataProviderNotStatic', + [ + $methodName, + ] + ); + } else { + $fix = $file->addFixableWarning( + 'Data provider method "%s" will need to be converted to static in future.', + $pointer + 2, + 'dataProviderNotStatic', + [ + $methodName, + ] + ); + + if ($fix) { + $file->fixer->beginChangeset(); + $file->fixer->addContentBefore($providerPointer, "static "); + $uses = self::findMethodCalls($file, $classPointer, $methodName); + foreach ($uses as $use) { + $file->fixer->replaceToken($use['start'], 'self::' . $methodName); + $file->fixer->replaceToken($use['start'] + 1, ''); + $file->fixer->replaceToken($use['start'] + 2, ''); + } + $file->fixer->endChangeset(); + } + } + } + } + + + /** + * Find all calls to a method. + * @param File $phpcsFile + * @param int $classPtr + * @param string $methodName + * @return array + */ + protected static function findMethodCalls( + File $phpcsFile, + int $classPtr, + string $methodName + ): array + { + $data = []; + + $mStart = $classPtr; + $tokens = $phpcsFile->getTokens(); + while ($mStart = $phpcsFile->findNext(T_VARIABLE, $mStart + 1, $tokens[$classPtr]['scope_closer'])) { + if ($tokens[$mStart]['content'] !== '$this') { + continue; + } + if ($tokens[$mStart + 1]['code'] !== T_OBJECT_OPERATOR) { + continue; + } + if ($tokens[$mStart + 2]['code'] !== T_STRING) { + continue; + } + if ($tokens[$mStart + 2]['content'] !== $methodName) { + continue; + } + + $data[] = [ + 'start' => $mStart, + 'end' => $mStart + 2, + ]; + } + + return $data; + } +} diff --git a/moodle/Tests/MoodleCSBaseTestCase.php b/moodle/Tests/MoodleCSBaseTestCase.php index a2dfaaf..30bf99b 100644 --- a/moodle/Tests/MoodleCSBaseTestCase.php +++ b/moodle/Tests/MoodleCSBaseTestCase.php @@ -83,11 +83,11 @@ public function set_component_mapping(array $mapping): void { protected function set_standard($standard) { $installedStandards = \PHP_CodeSniffer\Util\Standards::getInstalledStandardDetails(); - foreach (array_keys($installedStandards) as $standard) { - if (\PHP_CodeSniffer\Util\Standards::isInstalledStandard($standard) === false) { + foreach (array_keys($installedStandards) as $installedStandard) { + if (\PHP_CodeSniffer\Util\Standards::isInstalledStandard($installedStandard) === false) { // They didn't select a valid coding standard, so help them // out by letting them know which standards are installed. - $error = 'ERROR: the "'.$standard.'" coding standard is not installed. '; + $error = 'ERROR: the "'. $installedStandard.'" coding standard is not installed. '; ob_start(); \PHP_CodeSniffer\Util\Standards::printInstalledStandards(); $error .= ob_get_contents(); @@ -96,37 +96,6 @@ protected function set_standard($standard) { } } $this->standard = $standard; - return; - // Since 2.9 arbitrary standard directories are not allowed by default, - // only those under the CodeSniffer/Standards dir are detected. Other base - // dirs containing standards can be added using CodeSniffer.conf or the - // PHP_CODESNIFFER_CONFIG_DATA global (installed_paths setting). - // We are using the global way here to avoid changes in the phpcs import. - // phpcs:disable - if (!isset($GLOBALS['PHP_CODESNIFFER_CONFIG_DATA']['installed_paths'])) { - $localcodecheckerpath = realpath(__DIR__ . '/../'); - $GLOBALS['PHP_CODESNIFFER_CONFIG_DATA'] = ['installed_paths' => $localcodecheckerpath]; - } - // phpcs:enable - - // Basic search of standards in the allowed directories. - $stdsearch = array( - __DIR__ . '/../phpcs/src/Standards', // PHPCS standards dir. - __DIR__ . '/..', // Plugin local_codechecker dir, allowed above via global. - ); - - foreach ($stdsearch as $stdpath) { - $stdpath = realpath($stdpath . '/' . $standard); - $stdfile = $stdpath . '/ruleset.xml'; - if (file_exists($stdfile)) { - $this->standard = $stdpath; // Need to pass the path here. - break; - } - } - // Standard not found, fail. - if ($this->standard === null) { - $this->fail('Standard "' . $standard . '" not found.'); - } } /** diff --git a/moodle/Tests/MoodleUtilTest.php b/moodle/Tests/MoodleUtilTest.php index c49c5cf..318854c 100644 --- a/moodle/Tests/MoodleUtilTest.php +++ b/moodle/Tests/MoodleUtilTest.php @@ -466,4 +466,202 @@ protected function cleanMoodleUtilCaches() { $moodleComponents->setAccessible(true); $moodleComponents->setValue([]); } + + /** + * Data provider for testIsUnitTest. + * + * @return array + */ + public static function isUnitTestProvider(): array + { + return [ + 'Not in tests directory' => [ + 'value' => '/path/to/standard/file.php', + 'return' => false, + ], + 'In tests directory' => [ + 'value' => '/path/to/standard/tests/file.php', + 'return' => true, + ], + 'In test sub-directory' => [ + 'value' => '/path/to/standard/tests/sub/file.php', + 'return' => true, + ], + 'Generator' => [ + 'value' => '/path/to/standard/tests/generator/file.php', + 'return' => false, + ], + 'Fixture' => [ + 'value' => '/path/to/standard/tests/fixtures/file.php', + 'return' => false, + ], + 'Behat' => [ + 'value' => '/path/to/standard/tests/behat/behat_test_file.php', + 'return' => false, + ], + ]; + } + + /** + * @dataProvider isUnitTestProvider + */ + public function testIsUnitTest( + string $filepath, + bool $expected + ): void + { + $phpcsConfig = new Config(); + $phpcsRuleset = new Ruleset($phpcsConfig); + $file = new File($filepath, $phpcsRuleset, $phpcsConfig); + + $this->assertEquals($expected, MoodleUtil::isUnitTest($file)); + } + + /** + * Data provider for testMeetsMinimumMoodleVersion. + * + * @return array + */ + public static function meetsMinimumMoodleVersionProvider(): array + { + return [ + // Setting up moodleBranch config/runtime option. + 'moodleBranch_not_integer' => [ + 'moodleVersion' => 'noint', + 'minVersion' => 311, + 'return' => ['exception' => DeepExitException::class, 'message' => 'Value in not an integer'], + ], + 'moodleBranch_big' => [ + 'moodleVersion' => '10000', + 'minVersion' => 311, + 'return' => ['exception' => DeepExitException::class, 'message' => 'Value must be 4 digit max'], + ], + 'moodleBranch_valid_meets_minimum' => [ + 'moodleVersion' => 999, + 'minVersion' => 311, + 'return' => ['value' => true], + ], + 'moodleBranch_valid_equals_minimum' => [ + 'moodleVersion' => 311, + 'minVersion' => 311, + 'return' => ['value' => true], + ], + 'moodleBranch_valid_does_not_meet_minimum' => [ + 'moodleVersion' => 311, + 'minVersion' => 402, + 'return' => ['value' => false], + ], + 'moodleBranch_valid_but_empty' => [ + 'moodleVersion' => 0, + 'minVersion' => 311, + 'return' => ['value' => null], + ], + ]; + } + + /** + * @dataProvider meetsMinimumMoodleVersionProvider + * @param array $config + * @param int $version + * @param array $return + * @param bool $reset + * @param bool $selfPath + */ + public function testMeetsMinimumMoodleVersion( + int|string $moodleVersion, + int $minVersion, + array $return, + ): void + { + Config::setConfigData('moodleBranch', $moodleVersion, true); + + $phpcsConfig = new Config(); + $phpcsRuleset = new Ruleset($phpcsConfig); + $file = new File('/path/to/tests/file.php', $phpcsRuleset, $phpcsConfig); + + // Exception is coming, let's verify it happens. + if (isset($return['exception'])) { + try { + MoodleUtil::getMoodleBranch($file); + } catch (\Exception $e) { + $this->assertInstanceOf($return['exception'], $e); + $this->assertStringContainsString($return['message'], $e->getMessage()); + } + } else if (array_key_exists('value', $return)) { + // Normal asserting result. + $this->assertSame($return['value'], MoodleUtil::meetsMinimumMoodleVersion($file, $minVersion)); + } + + // Do we want to reset any information cached (by default we do). + $this->cleanMoodleUtilCaches(); + + // We need to unset all config options when passed. + Config::setConfigData('moodleBranch', null, true); + } + + public static function findClassMethodPointerProvider(): array + { + return [ + [ + 'instance_method', + true, + ], + [ + 'protected_method', + true, + ], + [ + 'private_method', + true, + ], + [ + 'static_method', + true, + ], + [ + 'protected_static_method', + true, + ], + [ + 'private_static_method', + true, + ], + [ + 'not_found_method', + false, + ], + ]; + } + + /** + * @dataProvider findClassMethodPointerProvider + */ + public function testFindClassMethodPointer( + string $methodName, + bool $found, + ): void + { + $phpcsConfig = new Config(); + $phpcsRuleset = new Ruleset($phpcsConfig); + $phpcsFile = new \PHP_CodeSniffer\Files\LocalFile( + __DIR__ . '/fixtures/moodleutil/test_with_methods_to_find.php', + $phpcsRuleset, + $phpcsConfig + ); + + $phpcsFile->process(); + $classPointer = $phpcsFile->findNext(T_CLASS, 0); + + $pointer = MoodleUtil::findClassMethodPointer( + $phpcsFile, + $classPointer, + $methodName + ); + + if ($found) { + $this->assertGreaterThan(0, $pointer); + } else { + $this->assertNull($pointer); + } + } } diff --git a/moodle/Tests/PHPUnitTestCaseProviderTest.php b/moodle/Tests/PHPUnitTestCaseProviderTest.php new file mode 100644 index 0000000..12e5e67 --- /dev/null +++ b/moodle/Tests/PHPUnitTestCaseProviderTest.php @@ -0,0 +1,152 @@ +. + +namespace MoodleHQ\MoodleCS\moodle\Tests; + +use MoodleHQ\MoodleCS\moodle\Util\MoodleUtil; + +// phpcs:disable moodle.NamingConventions + +/** + * Test the TestCaseCoversSniff 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\TestCaseProviderSniff + */ +class PHPUnitTestCaseProviderTest extends MoodleCSBaseTestCase { + + /** + * Data provider for self::test_phpunit_test_providers + */ + public function provider_phpunit_data_providers() { + return [ + 'Correct' => [ + 'fixture' => 'fixtures/phpunit/provider/correct_test.php', + 'errors' => [], + 'warnings' => [], + ], + 'Provider Casing' => [ + 'fixture' => 'fixtures/phpunit/provider/provider_casing_test.php', + 'errors' => [ + 6 => 'Wrong @dataProvider tag: @dataprovider provided, @dataProvider expected', + ], + 'warnings' => [ + ], + ], + 'Provider Visibility' => [ + 'fixture' => 'fixtures/phpunit/provider/provider_visibility_test.php', + 'errors' => [ + 12 => 'Data provider method "provider" must be public.', + 23 => 'Data provider method "provider_without_visibility" visibility should be specified.', + 34 => 'Data provider method "static_provider_without_visibility" visibility should be specified.', + ], + 'warnings' => [ + 17 => 'Data provider method "provider_without_visibility" will need to be converted to static in future.', + ], + ], + 'Provider Naming conflicts with test names' => [ + 'fixture' => 'fixtures/phpunit/provider/provider_prefix_test.php', + 'errors' => [ + 6 => 'Data provider must not start with "test_". "test_provider" provided.', + ], + 'warnings' => [ + ], + ], + 'Static Providers' => [ + 'fixture' => 'fixtures/phpunit/provider/static_providers_test.php', + 'errors' => [ + ], + 'warnings' => [ + 12 => 'Data provider method "fixable_provider" will need to be converted to static in future.', + 23 => 'Data provider method "unfixable_provider" will need to be converted to static in future.', + 34 => 'Data provider method "partially_fixable_provider" will need to be converted to static in future.', + ], + ], + 'Static Providers Applying fixes' => [ + 'fixture' => 'fixtures/phpunit/provider/static_providers_fix_test.php', + 'errors' => [ + ], + 'warnings' => [ + 13 => 'Data provider method "fixable_provider" will need to be converted to static in future.', + 24 => 'Data provider method "unfixable_provider" will need to be converted to static in future.', + 35 => 'Data provider method "partially_fixable_provider" will need to be converted to static in future.', + ], + ], + 'Provider Return Type checks' => [ + 'fixture' => 'fixtures/phpunit/provider/provider_returntype_test.php', + 'errors' => [ + 6 => 'Data provider method "provider_no_return" must return an array, a Generator or an Iterable.', + 17 => 'Data provider method "provider_wrong_return" must return an array, a Generator or an Iterable.', + 28 => 'Data provider method "provider_returns_generator" must return an array, a Generator or an Iterable.', + 41 => 'Data provider method "provider_returns_iterator" must return an array, a Generator or an Iterable.', + ], + 'warnings' => [ + ], + ], + 'Provider not found' => [ + 'fixture' => 'fixtures/phpunit/provider/provider_not_found_test.php', + 'errors' => [ + 6 => 'Data provider method "provider" not found.', + 14 => 'Wrong @dataProvider tag specified for test test_two, it must be followed by a space and a method name.', + ], + 'warnings' => [ + ], + ], + 'Complex test with multiple classes' => [ + 'fixture' => 'fixtures/phpunit/provider/complex_provider_test.php', + 'errors' => [ + 7 => 'Data provider method "provider" not found.', + ], + 'warnings' => [ + 14 => 'Data provider method "second_provider" will need to be converted to static in future.', + ], + ], + ]; + } + + /** + * 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_providers + */ + public function test_phpunit_test_providers( + string $fixture, + array $errors, + array $warnings + ): void { + // Define the standard, sniff and fixture to use. + $this->set_standard('moodle'); + $this->set_sniff('moodle.PHPUnit.TestCaseProvider'); + $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(); + } +} diff --git a/moodle/Tests/fixtures/moodleutil/test_with_methods_to_find.php b/moodle/Tests/fixtures/moodleutil/test_with_methods_to_find.php new file mode 100644 index 0000000..401aa91 --- /dev/null +++ b/moodle/Tests/fixtures/moodleutil/test_with_methods_to_find.php @@ -0,0 +1,23 @@ +second_provider(); + } +} diff --git a/moodle/Tests/fixtures/phpunit/provider/complex_provider_test.php.fixed b/moodle/Tests/fixtures/phpunit/provider/complex_provider_test.php.fixed new file mode 100644 index 0000000..890f08d --- /dev/null +++ b/moodle/Tests/fixtures/phpunit/provider/complex_provider_test.php.fixed @@ -0,0 +1,43 @@ +// phpcs:set moodle.PHPUnit.TestCaseProvider autofixStaticProviders true +second_provider(); + } +} diff --git a/moodle/Tests/fixtures/phpunit/provider/correct_test.php b/moodle/Tests/fixtures/phpunit/provider/correct_test.php new file mode 100644 index 0000000..99de326 --- /dev/null +++ b/moodle/Tests/fixtures/phpunit/provider/correct_test.php @@ -0,0 +1,22 @@ +provider(); + } + + /** + * @dataProvider partially_fixable_provider + */ + public function test_partially_fixable(): void { + // Nothing to test. + } + + public function partially_fixable_provider(): array { + $this->call_something(); + return $this->fixable_provider(); + } +} diff --git a/moodle/Tests/fixtures/phpunit/provider/static_providers_fix_test.php.fixed b/moodle/Tests/fixtures/phpunit/provider/static_providers_fix_test.php.fixed new file mode 100644 index 0000000..f10b107 --- /dev/null +++ b/moodle/Tests/fixtures/phpunit/provider/static_providers_fix_test.php.fixed @@ -0,0 +1,45 @@ +// phpcs:set moodle.PHPUnit.TestCaseProvider autofixStaticProviders true +provider(); + } + + /** + * @dataProvider partially_fixable_provider + */ + public function test_partially_fixable(): void { + // Nothing to test. + } + + public function partially_fixable_provider(): array { + $this->call_something(); + return self::fixable_provider(); + } +} diff --git a/moodle/Tests/fixtures/phpunit/provider/static_providers_test.php b/moodle/Tests/fixtures/phpunit/provider/static_providers_test.php new file mode 100644 index 0000000..2589c31 --- /dev/null +++ b/moodle/Tests/fixtures/phpunit/provider/static_providers_test.php @@ -0,0 +1,44 @@ +provider(); + } + + /** + * @dataProvider partially_fixable_provider + */ + public function test_partially_fixable(): void { + // Nothing to test. + } + + public function partially_fixable_provider(): array { + $this->call_something(); + return $this->fixable_provider(); + } +} diff --git a/moodle/Tests/fixtures/phpunit/provider/static_providers_test.php.fixed b/moodle/Tests/fixtures/phpunit/provider/static_providers_test.php.fixed new file mode 100644 index 0000000..2589c31 --- /dev/null +++ b/moodle/Tests/fixtures/phpunit/provider/static_providers_test.php.fixed @@ -0,0 +1,44 @@ +provider(); + } + + /** + * @dataProvider partially_fixable_provider + */ + public function test_partially_fixable(): void { + // Nothing to test. + } + + public function partially_fixable_provider(): array { + $this->call_something(); + return $this->fixable_provider(); + } +} diff --git a/moodle/Util/MoodleUtil.php b/moodle/Util/MoodleUtil.php index 2f10b92..5e8eb25 100644 --- a/moodle/Util/MoodleUtil.php +++ b/moodle/Util/MoodleUtil.php @@ -18,6 +18,7 @@ use PHP_CodeSniffer\Config; use PHP_CodeSniffer\Exceptions\DeepExitException; +use PHP_CodeSniffer\Exceptions\RuntimeException; use PHP_CodeSniffer\Files\DummyFile; use PHP_CodeSniffer\Files\File; use PHP_CodeSniffer\Ruleset; @@ -370,4 +371,107 @@ public static function getMoodleRoot(File $file = null, bool $selfPath = true) { self::$moodleRoot = null; return self::$moodleRoot; } + + /** + * Whether this file is a unit test file. + * + * This does not include test fixtures, generators, or behat files. + * + * Any file which is not correctly named will be ignored. + * + * @param File $phpcsFile + * @return bool + */ + public static function isUnitTest(File $phpcsFile): bool + { + // If the file isn't under tests directory, nothing to check. + if (stripos($phpcsFile->getFilename(), '/tests/') === false) { + return false; // @codeCoverageIgnore + } + + // If the file is in a fixture directory, ignore it. + if (stripos($phpcsFile->getFilename(), '/tests/fixtures/') !== false) { + return false; // @codeCoverageIgnore + } + + // If the file is in a generator directory, ignore it. + if (stripos($phpcsFile->getFilename(), '/tests/generator/') !== false) { + return false; // @codeCoverageIgnore + } + + // If the file is in a behat directory, ignore it. + if (stripos($phpcsFile->getFilename(), '/tests/behat/') !== false) { + return false; // @codeCoverageIgnore + } + + // If the file isn't called, _test.php, nothing to check. + // Make an exception for codechecker own phpunit fixtures here, allowing any name for them. + $fileName = basename($phpcsFile->getFilename()); + if (substr($fileName, -9) !== '_test.php' && !self::isUnitTestRunning()) { + return false; // @codeCoverageIgnore + } + + return true; + } + + /** + * Whether we are running PHPUnit. + * + * @return bool + * @codeCoverageIgnore + */ + public static function isUnitTestRunning(): bool + { + // Detect if we are running PHPUnit. + return defined('PHPUNIT_TEST') && PHPUNIT_TEST; + } + + /** + * Whether the file belongs to a version of Moodle meeting the specifeid minimum version. + * + * If a version could not be determined, null is returned. + * + * @param File $phpcsFile The file to check + * @param int The minimum version to check against as a 2, or 3 digit number. + * @return null|bool + */ + public static function meetsMinimumMoodleVersion( + File $phpcsFile, + int $version + ): ?bool + { + $moodleBranch = self::getMoodleBranch($phpcsFile); + if (!isset($moodleBranch)) { + // We cannot determine the moodle branch, so we cannot determine if the version is met. + return null; + } + + return ($moodleBranch >= $version); + } + + /** + * Find the pointer to a method in a class. + * + * @param File $phpcsFile + * @param int $classPtr + * @param string $methodName + * @return null|int + */ + public static function findClassMethodPointer( + File $phpcsFile, + int $classPtr, + string $methodName + ): ?int + { + $mStart = $classPtr; + $tokens = $phpcsFile->getTokens(); + while ($mStart = $phpcsFile->findNext(T_FUNCTION, $mStart + 1, $tokens[$classPtr]['scope_closer'])) { + $method = $phpcsFile->getDeclarationName($mStart); + if ($method === $methodName) { + return $mStart; + } + } + + return null; + } } diff --git a/moodle/ruleset.xml b/moodle/ruleset.xml index ac4628d..07402e7 100644 --- a/moodle/ruleset.xml +++ b/moodle/ruleset.xml @@ -90,6 +90,16 @@ + + + 0