Skip to content

Commit

Permalink
Add Error for empty setUp/tearDown methods
Browse files Browse the repository at this point in the history
  • Loading branch information
andrewnicols committed May 31, 2024
1 parent 6a458cb commit 4d1a7b4
Show file tree
Hide file tree
Showing 5 changed files with 39 additions and 16 deletions.
21 changes: 17 additions & 4 deletions moodle/Sniffs/PHPUnit/ParentSetUpTearDownSniff.php
Original file line number Diff line number Diff line change
Expand Up @@ -145,10 +145,23 @@ public function process(File $phpcsFile, $stackPtr): void {

// 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) {
// 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;
}

Expand Down
12 changes: 8 additions & 4 deletions moodle/Tests/Sniffs/PHPUnit/ParentSetUpTearDownSniffTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand All @@ -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' => [],
],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,3 +87,10 @@ public static function tearDownAfterClass(): void {
require('somefile.php');
}
}

class empty_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.
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,15 @@ defined('MOODLE_INTERNAL') || die(); // Make this always the 1st line in all CS

class missing_setup_teardown_test extends Something {

This comment has been minimized.

Copy link
@stronk7

stronk7 May 31, 2024

Only suggestion I can imagine… and without modifying line numbers is…

can we rename the test class to no_statement_setup_teardown_test (or so).

and then add some comment or phpdoc block in some of them.

with that, it’s perfect!

This comment has been minimized.

Copy link
@stronk7

stronk7 May 31, 2024

I've applied your commit with the changes proposed to moodlehq#165 . See you there.

public function setUp(): void {
parent::setUp();
}

public function tearDown(): void {
parent::tearDown();
}

public static function setUpBeforeClass(): void {
parent::setUpBeforeClass();
}

public static function tearDownAfterClass(): void {
parent::tearDownAfterClass();
}
}

Expand Down Expand Up @@ -95,3 +91,10 @@ class best_insertion_setup_teardown_test extends Something {
parent::tearDownAfterClass();
}
}

class empty_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.
}

0 comments on commit 4d1a7b4

Please sign in to comment.