From 8b2a06073a0a0bb279f7bf5bbb9a7a462484a019 Mon Sep 17 00:00:00 2001 From: "Eloy Lafuente (stronk7)" Date: Sun, 28 Nov 2021 23:10:12 +0100 Subject: [PATCH] New moodle.PHPUnit.TestCaseNames Sniff Sniff in charge of verifying various things related with PHPUnit test cases names (files and classes and namespaces). Like: - Error: _test.php files missing any valid test case. - Error: incorrectly names test cases (not ended with _test[case] - Warning: test case classes not matching test case file names. - Error: found duplicate test case names. - Error: found test case name proposed for another test case. - Error: test case namespace not matching expected component namespace. - Warning: test case namespace missing, suggesting a correct one. - Error: found duplicate proposed test case names. - Error: found proposed test case name existing for another test case. Practically covered with unit tests. And behat tests updated. Also included, small modification to local_codechecker_testcase base class that now, when an incorrect fixture file is passed, it fails with error (previously it was exiting silently and was hard to detect what was happening when a typo in the fixture name was used). --- moodle/Sniffs/PHPUnit/TestCaseNamesSniff.php | 271 ++++++++++++++++++ .../testcasenames_duplicate_exists.php | 11 + .../testcasenames_duplicate_proposed.php | 10 + .../phpunit/testcasenames_exists_proposed.php | 10 + .../phpunit/testcasenames_missing.php | 21 ++ .../phpunit/testcasenames_missing_ns.php | 10 + .../phpunit/testcasenames_nomatch.php | 11 + .../phpunit/testcasenames_proposed_exists.php | 11 + .../testcasenames_test_testcase_irregular.php | 11 + .../phpunit/testcasenames_unexpected_ns.php | 11 + moodle/tests/phpunit_testcasenames_test.php | 155 ++++++++++ tests/behat/ui.feature | 4 +- tests/local_codechecker_testcase.php | 3 + 13 files changed, 537 insertions(+), 2 deletions(-) create mode 100644 moodle/Sniffs/PHPUnit/TestCaseNamesSniff.php create mode 100644 moodle/tests/fixtures/phpunit/testcasenames_duplicate_exists.php create mode 100644 moodle/tests/fixtures/phpunit/testcasenames_duplicate_proposed.php create mode 100644 moodle/tests/fixtures/phpunit/testcasenames_exists_proposed.php create mode 100644 moodle/tests/fixtures/phpunit/testcasenames_missing.php create mode 100644 moodle/tests/fixtures/phpunit/testcasenames_missing_ns.php create mode 100644 moodle/tests/fixtures/phpunit/testcasenames_nomatch.php create mode 100644 moodle/tests/fixtures/phpunit/testcasenames_proposed_exists.php create mode 100644 moodle/tests/fixtures/phpunit/testcasenames_test_testcase_irregular.php create mode 100644 moodle/tests/fixtures/phpunit/testcasenames_unexpected_ns.php create mode 100644 moodle/tests/phpunit_testcasenames_test.php diff --git a/moodle/Sniffs/PHPUnit/TestCaseNamesSniff.php b/moodle/Sniffs/PHPUnit/TestCaseNamesSniff.php new file mode 100644 index 00000000..90eead4c --- /dev/null +++ b/moodle/Sniffs/PHPUnit/TestCaseNamesSniff.php @@ -0,0 +1,271 @@ +. + +/** + * Checks that a test file has a class name matching the file name. + * + * @package local_codechecker + * @copyright 2021 onwards Eloy Lafuente (stronk7) {@link https://stronk7.com} + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +namespace MoodleCodeSniffer\moodle\Sniffs\PHPUnit; + +// phpcs:disable moodle.NamingConventions + +use PHP_CodeSniffer\Sniffs\Sniff; +use PHP_CodeSniffer\Files\File; +use PHP_CodeSniffer\Util\Tokens; +use MoodleCodeSniffer\moodle\Util\MoodleUtil; + +class TestCaseNamesSniff implements Sniff { + + /** + * List of classes that have been found during checking. + * + * @var array + */ + protected $foundClasses = []; + + /** + * List of classes that have been proposed during checking. + * + * @var array + */ + protected $proposedClasses = []; + + /** + * Register for open tag (only process once per file). + */ + public function register() { + return array(T_OPEN_TAG); + } + + /** + * Processes php files and perform various checks with file, namespace and class names. + * inclusion. + * + * @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. + + // Guess moodle component (from $file being processed). + $moodleComponent = MoodleUtil::getMoodleComponent($file); + + // 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; + } + + // If the file isn't under tests directory, nothing to check. + if (strpos($file->getFilename(), '/tests/') === false) { + return; + } + + // If the file isn't called, _test.php, nothing to check. + $fileName = basename($file->getFilename()); + if (substr($fileName, -9) !== '_test.php') { + // Make an exception for codechecker own phpunit fixtures here, allowing any name for them. + if (!defined('PHPUNIT_TEST') || !PHPUNIT_TEST) { + return; + } + } + + // In order to cover the duplicates detection, we need to set some + // properties (caches) here. It's extremely hard to do + // this via mocking / extending (at very least for this humble developer). + if (defined('PHPUNIT_TEST') && PHPUNIT_TEST) { + $this->prepareCachesForPHPUnit(); + } + + // Get the class namespace. + $namespace = ''; + $nsStart = 0; + if ($nsStart = $file->findNext(T_NAMESPACE, ($pointer + 1))) { + $nsEnd = $file->findNext([T_NS_SEPARATOR, T_STRING, T_WHITESPACE], ($nsStart + 1), null, true); + $namespace = strtolower(trim($file->getTokensAsString(($nsStart + 1), ($nsEnd - $nsStart - 1)))); + } + $pointer = $nsEnd ?? $pointer; // When possible, move the pointer to after the namespace name. + + // Get the name of the 1st class in the file (this Sniff doesn't detects multiple), + // verify that it extends something and that has a test_ method. + $class = ''; + $classFound = false; + while ($cStart = $file->findNext(T_CLASS, $pointer)) { + $pointer = $cStart + 1; // Move the pointer to the class start. + + // 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; + } + + // Verify that the class has some test_xxx method. + $method = ''; + $methodFound = false; + while ($mStart = $file->findNext(T_FUNCTION, $pointer, $tokens[$cStart]['scope_closer'])) { + $pointer = $tokens[$mStart]['scope_closer']; // Next iteration look after the end of current method. + if (strpos($file->getDeclarationName($mStart), 'test_') === 0) { + $methodFound = true; + $method = $file->getDeclarationName($mStart); + break; + } + } + + // If we have found a test_ method, this is our class (the 1st having one). + if ($methodFound) { + $classFound = true; + $class = $file->getDeclarationName($cStart); + $class = strtolower(trim($class)); + break; + } + $pointer = $tokens[$cStart]['scope_closer']; // Move the pointer to the class end. + } + + // No testcase class found, this is plain-wrong. + if (!$classFound) { + $file->addError('PHPUnit test file missing any valid testcase class declaration', 0, 'Missing'); + return; // If arrived here we don't have a valid class, we are finished. + } + + // All the following checks assume that a valid class has been found. + + // Error if the found classname is "strange" (not "_test|_testcase" ended). + if (substr($class, -5) !== '_test' && substr($class, -9) != '_testcase') { + $file->addError('PHPUnit irregular testcase name found: %s (_test/_testcase ended expected)', $cStart, + 'Irregular', [$class]); + } + + // Check if the file name and the class name match, warn if not. + $baseName = pathinfo($fileName, PATHINFO_FILENAME); + if ($baseName !== $class) { + $file->addWarning('PHPUnit testcase name "%s" does not match file name "%s"', $cStart, + 'NoMatch', [$class, $baseName]); + } + + // Check if the class has been already found (this is useful when running against a lot of files). + $fdqnClass = $namespace ? $namespace . '\\' . $class : $class; + if (isset($this->foundClasses[$fdqnClass])) { + // Already found, this is a dupe class name, error! + foreach ($this->foundClasses[$fdqnClass] as $exists) { + $file->addError('PHPUnit testcase "%s" already exists at "%s" line %s', $cStart, + 'DuplicateExists', [$fdqnClass, $exists['file'], $exists['line']]); + } + } else { + // Create the empty element. + $this->foundClasses[$fdqnClass] = []; + } + + // Add the new element. + $this->foundClasses[$fdqnClass][] = [ + 'file' => $file->getFilename(), + 'line' => $tokens[$cStart]['line'], + ]; + + // Check if the class has been already proposed (this is useful when running against a lot of files). + if (isset($this->proposedClasses[$fdqnClass])) { + // Already found, this is a dupe class name, error! + foreach ($this->proposedClasses[$fdqnClass] as $exists) { + $file->addError('PHPUnit testcase "%s" already proposed for "%s" line %s. You ' . + 'may want to change the testcase name (file and class)', $cStart, + 'ProposedExists', [$fdqnClass, $exists['file'], $exists['line']]); + } + } + + // Validate 1st level namespace. + + if ($namespace && $moodleComponent) { + // Verify that the namespace declared in the class matches the namespace expected for the file. + if (strpos($namespace . '\\', $moodleComponent . '\\') !== 0) { + $file->addError('PHPUnit class namespace "%s" does not match expected file namespace "%s"', $nsStart, + 'UnexpectedNS', [$namespace, $moodleComponent]); + } + } + + if (!$namespace && $moodleComponent) { + $file->addWarning('PHUnit class "%s" does not have any namespace. It is recommended to add it to the "%s" ' . + 'namespace, using more levels if needed, in order to match the code being tested', $cStart, + 'MissingNS', [$fdqnClass, $moodleComponent]); + + // Check if the proposed class has been already proposed (this is useful when running against a lot of files). + $fdqnProposed = $moodleComponent . '\\' . $fdqnClass; + if (isset($this->proposedClasses[$fdqnProposed])) { + // Already found, this is a dupe class name, error! + foreach ($this->proposedClasses[$fdqnProposed] as $exists) { + $file->addError('Proposed PHPUnit testcase "%s" already proposed for "%s" line %s. You ' . + 'may want to change the testcase name (file and class)', $cStart, + 'DuplicateProposed', [$fdqnProposed, $exists['file'], $exists['line']]); + } + } else { + // Create the empty element. + $this->proposedClasses[$fdqnProposed] = []; + } + + // Add the new element. + $this->proposedClasses[$fdqnProposed][] = [ + 'file' => $file->getFilename(), + 'line' => $tokens[$cStart]['line'], + ]; + + // Check if the proposed class has been already found (this is useful when running against a lot of files). + if (isset($this->foundClasses[$fdqnProposed])) { + // Already found, this is a dupe class name, error! + foreach ($this->foundClasses[$fdqnProposed] as $exists) { + $file->addError('Proposed PHPUnit testcase "%s" already exists at "%s" line %s. You ' . + 'may want to change the testcase name (file and class)', $cStart, + 'ExistsProposed', [$fdqnProposed, $exists['file'], $exists['line']]); + } + } + } + } + + /** + * Prepare found and proposed caches for PHPUnit. + * + * It's near impossible to extend or mock this class from PHPUnit in order + * to get the caches pre-filled with some values that will cover some + * of the logic of the sniff (at least for this developer). + * + * So we fill them here when it's detected that we are running PHPUnit. + */ + private function prepareCachesForPHPUnit() { + $this->foundClasses['local_codechecker\testcasenames_duplicate_exists'][] = [ + 'file' => 'phpunit_fake_exists', + 'line' => -999, + ]; + $this->foundClasses['local_codechecker\testcasenames_exists_proposed'][] = [ + 'file' => 'phpunit_fake_exists', + 'line' => -999, + ]; + $this->proposedClasses['local_codechecker\testcasenames_duplicate_proposed'][] = [ + 'file' => 'phpunit_fake_proposed', + 'line' => -999, + ]; + $this->proposedClasses['local_codechecker\testcasenames_proposed_exists'][] = [ + 'file' => 'phpunit_fake_proposed', + 'line' => -999, + ]; + } +} diff --git a/moodle/tests/fixtures/phpunit/testcasenames_duplicate_exists.php b/moodle/tests/fixtures/phpunit/testcasenames_duplicate_exists.php new file mode 100644 index 00000000..b84fd924 --- /dev/null +++ b/moodle/tests/fixtures/phpunit/testcasenames_duplicate_exists.php @@ -0,0 +1,11 @@ +. + +namespace local_codechecker; + +use MoodleCodeSniffer\moodle\Util\MoodleUtil; + +defined('MOODLE_INTERNAL') || die(); + +require_once(__DIR__ . '/../../tests/local_codechecker_testcase.php'); +require_once(__DIR__ . '/../Util/MoodleUtil.php'); + +// phpcs:disable moodle.NamingConventions + +/** + * Test the TestCaseNamesSniff sniff. + * + * @package local_codechecker + * @category test + * @copyright 2021 onwards Eloy Lafuente (stronk7) {@link http://stronk7.com} + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * + * @covers \MoodleCodeSniffer\moodle\Sniffs\PHPUnit\TestCaseNamesSniff + */ +class phpunit_testcasenames_test extends local_codechecker_testcase { + + /** + * Data provider for self::test_phpunit_testcasenames + */ + public function provider_phpunit_testcasenames() { + return [ + 'Missing' => [ + 'fixture' => 'fixtures/phpunit/testcasenames_missing.php', + 'errors' => [ + 1 => '@Message: PHPUnit test file missing any valid testcase class', + ], + 'warnings' => [], + ], + 'Irregular' => [ + 'fixture' => 'fixtures/phpunit/testcasenames_test_testcase_irregular.php', + 'errors' => [ + 8 => '@Message: PHPUnit irregular testcase name found: testcasenames_test_testcase_irregular', + ], + 'warnings' => [], + ], + 'NoMatch' => [ + 'fixture' => 'fixtures/phpunit/testcasenames_nomatch.php', + 'errors' => [], + 'warnings' => [ + 8 => '"testcasenames_nomatch_test" does not match file name "testcasenames_nomatch"', + ], + ], + 'UnexpectedNS' => [ + 'fixture' => 'fixtures/phpunit/testcasenames_unexpected_ns.php', + 'errors' => [ + 2 => 'namespace "local_wrong" does not match expected file namespace "local_codechecker"', + 8 => 1, + ], + 'warnings' => [], + ], + 'MissingNS' => [ + 'fixture' => 'fixtures/phpunit/testcasenames_missing_ns.php', + 'errors' => [ + 7 => 1, + ], + 'warnings' => [ + 7 => 'add it to the "local_codechecker" namespace, using more levels', + ], + ], + 'DuplicateExists' => [ + 'fixture' => 'fixtures/phpunit/testcasenames_duplicate_exists.php', + 'errors' => [ + 8 => [ + 'irregular testcase name found', + 'testcasenames_duplicate_exists" already exists at "phpunit_fake_exists"', + ], + ], + 'warnings' => [], + ], + 'ProposedExists' => [ + 'fixture' => 'fixtures/phpunit/testcasenames_proposed_exists.php', + 'errors' => [ + 8 => [ + 'irregular testcase name found', + 'testcasenames_proposed_exists" already proposed for "phpunit_fake_proposed"', + ], + ], + 'warnings' => [], + ], + 'DuplicateProposed' => [ + 'fixture' => 'fixtures/phpunit/testcasenames_duplicate_proposed.php', + 'errors' => [ + 7 => [ + 'irregular testcase name found', + 'testcasenames_duplicate_proposed" already proposed for "phpunit_fake_proposed"', + ], + ], + 'warnings' => [ + 7 => 1 + ], + ], + 'ExistsProposed' => [ + 'fixture' => 'fixtures/phpunit/testcasenames_exists_proposed.php', + 'errors' => [ + 7 => [ + 'irregular testcase name found', + 'testcasenames_exists_proposed" already exists at "phpunit_fake_exists"', + ], + ], + 'warnings' => [ + 7 => 1 + ], + ], + ]; + } + + /** + * Test the moodle.PHPUnit.TestCaseNames 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_testcasenames + */ + public function test_phpunit_testcasenames(string $fixture, array $errors, array $warnings) { + + // Define the standard, sniff and fixture to use. + $this->set_standard('moodle'); + $this->set_sniff('moodle.PHPUnit.TestCaseNames'); + $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/tests/behat/ui.feature b/tests/behat/ui.feature index c0777a3f..9a845712 100644 --- a/tests/behat/ui.feature +++ b/tests/behat/ui.feature @@ -40,9 +40,9 @@ Feature: Codechecker UI works as expected Examples: | path | exclude | seen | notseen | - | local/codechecker/moodle/tests | */tests/fixtures/* | Files found: 2 | Invalid path | + | local/codechecker/moodle/tests | */tests/fixtures/* | Files found: 3 | Invalid path | | local/codechecker/moodle/tests | */tests/fixtures/* | moodlestandard_test.php | Invalid path | - | local/codechecker/moodle/tests/ | *PHPC*, *moodle_* | Files found: 18 | Invalid path | + | local/codechecker/moodle/tests/ | *PHPC*, *moodle_* | Files found: 28 | Invalid path | | local/codechecker/moodle/tests/ | *PHPC*, *moodle_* | Line 1 of the opening comment | moodle_php | | local/codechecker/moodle/tests/ | *PHPC*, *moodle_* | Inline comments must end | /phpcompat | | local/codechecker/moodle/tests/ | *PHPC*, *moodle_* | Inline comments must end | /phpcompat | diff --git a/tests/local_codechecker_testcase.php b/tests/local_codechecker_testcase.php index 34338adb..5d1dd2fe 100644 --- a/tests/local_codechecker_testcase.php +++ b/tests/local_codechecker_testcase.php @@ -161,6 +161,9 @@ protected function set_sniff($sniff) { * @param string $fixture full path to the file used as input (fixture). */ protected function set_fixture($fixture) { + if (!is_readable($fixture)) { + $this->fail('Unreadable fixture passed: '. $fixture); + } $this->fixture = $fixture; }