Skip to content

Commit 622f445

Browse files
andrewnicolsandrewnicols
committed
PHPUnit tests should have a return type
1 parent 6bb9012 commit 622f445

File tree

11 files changed

+469
-4
lines changed

11 files changed

+469
-4
lines changed

‎moodle/Sniffs/PHPUnit/TestReturnTypeSniff.php‎

Lines changed: 156 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,156 @@
1+
<?php
2+
// This file is part of Moodle - http://moodle.org/
3+
//
4+
// Moodle is free software: you can redistribute it and/or modify
5+
// it under the terms of the GNU General Public License as published by
6+
// the Free Software Foundation, either version 3 of the License, or
7+
// (at your option) any later version.
8+
//
9+
// Moodle is distributed in the hope that it will be useful,
10+
// but WITHOUT ANY WARRANTY; without even the implied warranty of
11+
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
12+
// GNU General Public License for more details.
13+
//
14+
// You should have received a copy of the GNU General Public License
15+
// along with Moodle. If not, see <http://www.gnu.org/licenses/>.
16+
17+
namespace MoodleHQ\MoodleCS\moodle\Sniffs\PHPUnit;
18+
19+
// phpcs:disable moodle.NamingConventions
20+
21+
use MoodleHQ\MoodleCS\moodle\Util\MoodleUtil;
22+
use PHP_CodeSniffer\Sniffs\Sniff;
23+
use PHP_CodeSniffer\Files\File;
24+
use PHPCSUtils\Utils\FunctionDeclarations;
25+
use PHPCSUtils\Utils\ObjectDeclarations;
26+
27+
/**
28+
* Checks that a test file has the @coversxxx annotations properly defined.
29+
*
30+
* @package local_codechecker
31+
* @copyright 2022 onwards Eloy Lafuente (stronk7) {@link https://stronk7.com}
32+
* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
33+
*/
34+
class TestReturnTypeSniff implements Sniff
35+
{
36+
37+
/**
38+
* Register for open tag (only process once per file).
39+
*/
40+
public function register() {
41+
return [
42+
T_OPEN_TAG,
43+
];
44+
}
45+
46+
/**
47+
* Processes php files and perform various checks with file.
48+
*
49+
* @param File $file The file being scanned.
50+
* @param int $pointer The position in the stack.
51+
*/
52+
public function process(File $file, $pointer)
53+
{
54+
// Before starting any check, let's look for various things.
55+
56+
// If we aren't checking Moodle 4.4dev (404) and up, nothing to check.
57+
// Make and exception for codechecker phpunit tests, so they are run always.
58+
if (!MoodleUtil::meetsMinimumMoodleVersion($file, 404) && !MoodleUtil::isUnitTestRunning()) {
59+
return; // @codeCoverageIgnore
60+
}
61+
62+
// If the file is not a unit test file, nothing to check.
63+
if (!MoodleUtil::isUnitTest($file) && !MoodleUtil::isUnitTestRunning()) {
64+
return; // @codeCoverageIgnore
65+
}
66+
67+
// We have all we need from core, let's start processing the file.
68+
69+
// Get the file tokens, for ease of use.
70+
$tokens = $file->getTokens();
71+
72+
// We only want to do this once per file.
73+
$prevopentag = $file->findPrevious(T_OPEN_TAG,
74+
$pointer - 1
75+
);
76+
if ($prevopentag !== false) {
77+
return; // @codeCoverageIgnore
78+
}
79+
80+
// Iterate over all the classes (hopefully only one, but that's not this sniff problem).
81+
$cStart = $pointer;
82+
while ($cStart = $file->findNext(T_CLASS, $cStart + 1)) {
83+
if (MoodleUtil::isUnitTestCaseClass($file, $cStart) === false) {
84+
// This class does not related to a uit test.
85+
continue;
86+
}
87+
88+
$classInfo = ObjectDeclarations::getClassProperties($file, $cStart);
89+
90+
// Iterate over all the methods in the class.
91+
$mStart = $cStart;
92+
while ($mStart = $file->findNext(T_FUNCTION, $mStart + 1, $tokens[$cStart]['scope_closer'])) {
93+
$method = $file->getDeclarationName($mStart);
94+
95+
// Ignore non test_xxxx() methods.
96+
if (strpos($method, 'test_') !== 0) {
97+
continue;
98+
}
99+
100+
// Get the function declaration.
101+
$functionInfo = FunctionDeclarations::getProperties($file, $mStart);
102+
103+
// 'return_type_token' => int|false, // The stack pointer to the start of the return type.
104+
if ($functionInfo['return_type_token'] !== false) {
105+
// This method has a return type.
106+
// In most cases that will be void, but it could be anything in the
107+
// case of chained or dependant tests.
108+
// TODO: Detect all cases of chained tests and dependant tests.
109+
continue;
110+
}
111+
112+
$methodNamePtr = $file->findNext(T_STRING, $mStart + 1, $tokens[$mStart]['parenthesis_opener']);
113+
$methodEnd = $file->findNext(T_CLOSE_PARENTHESIS, $mStart + 1);
114+
115+
// Detect if the method has a return statement.
116+
// If it does, then see if it has a value or not.
117+
$hasReturn = $file->findNext(T_RETURN, $mStart + 1, $tokens[$mStart]['scope_closer']);
118+
$probablyVoid = !$hasReturn;
119+
if ($hasReturn) {
120+
$next = $file->findNext(
121+
T_WHITESPACE,
122+
$hasReturn + 1,
123+
$tokens[$mStart]['scope_closer'],
124+
true
125+
);
126+
if ($tokens[$next]['code'] === T_SEMICOLON) {
127+
$probablyVoid = true;
128+
}
129+
}
130+
131+
$fix = false;
132+
if ($probablyVoid) {
133+
$fix = $file->addFixableWarning(
134+
'Test method %s() is missing a return type',
135+
$methodNamePtr,
136+
'MissingReturnType',
137+
[$method]
138+
);
139+
} else {
140+
$file->addWarning(
141+
'Test method %s() is missing a return type',
142+
$methodNamePtr,
143+
'MissingReturnType',
144+
[$method]
145+
);
146+
}
147+
148+
if ($fix) {
149+
$file->fixer->beginChangeset();
150+
$file->fixer->addContent($methodEnd, ': void');
151+
$file->fixer->endChangeset();
152+
}
153+
}
154+
}
155+
}
156+
}

‎moodle/Tests/MoodleUtilTest.php‎

Lines changed: 70 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
use PHP_CodeSniffer\Files\File;
2323
use PHP_CodeSniffer\Ruleset;
2424
use org\bovigo\vfs\vfsStream;
25+
use PHPCSUtils\Utils\ObjectDeclarations;
2526

2627
// phpcs:disable moodle.NamingConventions
2728

@@ -470,7 +471,7 @@ protected function cleanMoodleUtilCaches() {
470471
/**
471472
* Data provider for testIsUnitTest.
472473
*
473-
* @return array
474+
* @return array
474475
*/
475476
public static function isUnitTestProvider(): array
476477
{
@@ -529,6 +530,74 @@ public function testIsUnitTest(
529530
$this->assertEquals($expected, MoodleUtil::isUnitTest($file));
530531
}
531532

533+
/**
534+
* Data provider for testIsUnitTest.
535+
*
536+
* @return array
537+
*/
538+
public static function isUnitTestCaseClassProvider(): array {
539+
return [
540+
'Not in tests directory' => [
541+
'value' => '/path/to/standard/file_test.php',
542+
'testClassName' => 'irrelevant',
543+
'return' => false,
544+
],
545+
'testcase in a test file' => [
546+
'value' => __DIR__ . '/fixtures/moodleutil/istestcaseclass/multiple_classes_test.php',
547+
'testClassName' => 'is_testcase',
548+
'return' => true,
549+
],
550+
'not a testcase in a test file' => [
551+
'value' => __DIR__ . '/fixtures/moodleutil/istestcaseclass/multiple_classes_test.php',
552+
'testClassName' => 'not_a_test_class',
553+
'return' => false,
554+
],
555+
'not a testcase with test in the name in a test file' => [
556+
'value' => __DIR__ . '/fixtures/moodleutil/istestcaseclass/multiple_classes_test.php',
557+
'testClassName' => 'some_other_class_with_test_in_name',
558+
'return' => false,
559+
],
560+
'not a testcase but extends a test class in a test file' => [
561+
'value' => __DIR__ . '/fixtures/moodleutil/istestcaseclass/multiple_classes_test.php',
562+
'testClassName' => 'some_other_class_with_test_in_name',
563+
'return' => false,
564+
],
565+
'looks like a test but does not extend a test class' => [
566+
'value' => __DIR__ . '/fixtures/moodleutil/istestcaseclass/multiple_classes_test.php',
567+
'testClassName' => 'some_other_class_with_test_in_name_not_extending_test',
568+
'return' => false,
569+
],
570+
];
571+
}
572+
573+
/**
574+
* @dataProvider isUnitTestCaseClassProvider
575+
*/
576+
public function testIsUnitTestCaseClass(
577+
string $filepath,
578+
string $testClassName,
579+
bool $expected
580+
): void
581+
{
582+
$phpcsConfig = new Config();
583+
$phpcsRuleset = new Ruleset($phpcsConfig);
584+
585+
if (file_exists($filepath)) {
586+
$file = new \PHP_CodeSniffer\Files\LocalFile($filepath, $phpcsRuleset, $phpcsConfig);
587+
$file->process();
588+
} else {
589+
$file = new File($filepath, $phpcsRuleset, $phpcsConfig);
590+
}
591+
592+
$cStart = 0;
593+
while ($cStart = $file->findNext(T_CLASS, $cStart + 1)) {
594+
if (ObjectDeclarations::getName($file, $cStart) === $testClassName) {
595+
$this->assertEquals($expected, MoodleUtil::isUnitTestCaseClass($file, $cStart));
596+
break;
597+
}
598+
}
599+
}
600+
532601
/**
533602
* Data provider for testMeetsMinimumMoodleVersion.
534603
*

‎moodle/Tests/PHPUnitTestReturnTypeTest.php‎

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
<?php
2+
// This file is part of Moodle - http://moodle.org/
3+
//
4+
// Moodle is free software: you can redistribute it and/or modify
5+
// it under the terms of the GNU General Public License as published by
6+
// the Free Software Foundation, either version 3 of the License, or
7+
// (at your option) any later version.
8+
//
9+
// Moodle is distributed in the hope that it will be useful,
10+
// but WITHOUT ANY WARRANTY; without even the implied warranty of
11+
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
12+
// GNU General Public License for more details.
13+
//
14+
// You should have received a copy of the GNU General Public License
15+
// along with Moodle. If not, see <http://www.gnu.org/licenses/>.
16+
17+
namespace MoodleHQ\MoodleCS\moodle\Tests;
18+
19+
// phpcs:disable moodle.NamingConventions
20+
21+
/**
22+
* Test the PHPUnitTestReturnTypeSniff sniff.
23+
*
24+
* @package local_codechecker
25+
* @category test
26+
* @copyright 2022 onwards Eloy Lafuente (stronk7) {@link http://stronk7.com}
27+
* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
28+
*
29+
* @covers \MoodleHQ\MoodleCS\moodle\Sniffs\PHPUnit\TestReturnTypeSniff
30+
*/
31+
class PHPUnitTestReturnTypeTest extends MoodleCSBaseTestCase {
32+
33+
/**
34+
* Data provider for self::provider_phpunit_data_returntypes
35+
*/
36+
public function provider_phpunit_data_returntypes(): array {
37+
return [
38+
'Provider Casing' => [
39+
'fixture' => 'fixtures/phpunit/TestReturnType/returntypes.php',
40+
'errors' => [
41+
],
42+
'warnings' => [
43+
6 => 'Test method test_one() is missing a return type',
44+
27 => 'Test method test_with_a_return() is missing a return type',
45+
32 => 'Test method test_with_another_return() is missing a return type',
46+
38 => 'Test method test_with_empty_return() is missing a return type',
47+
],
48+
],
49+
];
50+
}
51+
52+
/**
53+
* Test the moodle.PHPUnit.TestCaseCovers sniff
54+
*
55+
* @param string $fixture relative path to fixture to use.
56+
* @param array $errors array of errors expected.
57+
* @param array $warnings array of warnings expected.
58+
* @dataProvider provider_phpunit_data_returntypes
59+
*/
60+
public function test_phpunit_test_returntypes(
61+
string $fixture,
62+
array $errors,
63+
array $warnings
64+
): void {
65+
// Define the standard, sniff and fixture to use.
66+
$this->set_standard('moodle');
67+
$this->set_sniff('moodle.PHPUnit.TestReturnType');
68+
$this->set_fixture(__DIR__ . '/' . $fixture);
69+
70+
// Define expected results (errors and warnings). Format, array of:
71+
// - line => number of problems, or
72+
// - line => array of contents for message / source problem matching.
73+
// - line => string of contents for message / source problem matching (only 1).
74+
$this->set_errors($errors);
75+
$this->set_warnings($warnings);
76+
77+
// Let's do all the hard work!
78+
$this->verify_cs_results();
79+
}
80+
}

‎moodle/Tests/Sniffs/Namespaces/NamespaceStatementSniffTest.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@
2828
* @copyright 2023 Andrew Lyons <[email protected]>
2929
* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
3030
*
31-
* @covers \MoodleHQ\MoodleCS\moodle\Sniffs\Namespaces\NoLeadingSlashSniff
31+
* @covers \MoodleHQ\MoodleCS\moodle\Sniffs\Namespaces\NamespaceStatementSniff
3232
*/
3333
class NamespaceStatementSniffTest extends MoodleCSBaseTestCase
3434
{

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
<?php
2+
defined('MOODLE_INTERNAL') || die(); // Make this always the 1st line in all CS fixtures.
3+
4+
class is_testcase extends base_test {
5+
}

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
<?php
2+
defined('MOODLE_INTERNAL') || die(); // Make this always the 1st line in all CS fixtures.
3+
4+
class is_testcase extends base_test {
5+
}
6+
7+
class not_a_test_class extends base_test {
8+
}
9+
10+
class some_other_class_with_test_in_name extends base_test {
11+
}
12+
13+
class some_other_class_with_test_in_name_not_extending_test {
14+
}

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
<?php
2+
defined('MOODLE_INTERNAL') || die(); // Make this always the 1st line in all CS fixtures.
3+
4+
class not_a_test_case_class extends base_test {
5+
}

0 commit comments

Comments
 (0)