From 64047f9fe0b8363aaf51803b9b61ef1d7faba7f0 Mon Sep 17 00:00:00 2001
From: "Eloy Lafuente (stronk7)" <stronk7@moodle.org>
Date: Tue, 28 May 2024 08:44:41 +0200
Subject: [PATCH] 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
---
 .../PHPUnit/ParentSetUpTearDownSniff.php      | 222 ++++++++++++++++++
 .../PHPUnit/ParentSetUpTearDownSniffTest.php  |  83 +++++++
 .../fixtures/ParentSetUpTearDownCorrect.php   |  69 ++++++
 .../fixtures/ParentSetUpTearDownProblems.php  |  89 +++++++
 .../ParentSetUpTearDownProblems.php.fixed     |  97 ++++++++
 5 files changed, 560 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/moodle/Sniffs/PHPUnit/ParentSetUpTearDownSniff.php b/moodle/Sniffs/PHPUnit/ParentSetUpTearDownSniff.php
new file mode 100644
index 00000000..09a5de19
--- /dev/null
+++ b/moodle/Sniffs/PHPUnit/ParentSetUpTearDownSniff.php
@@ -0,0 +1,222 @@
+<?php
+
+// This file is part of Moodle - https://moodle.org/
+//
+// Moodle is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// Moodle is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with Moodle.  If not, see <https://www.gnu.org/licenses/>.
+
+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[]
+     */
+    private static array $setUpMethods = [
+        'setUp',
+        'setUpBeforeClass',
+    ];
+
+    /**
+     * @var string[]
+     */
+    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 @@
+<?php
+
+// This file is part of Moodle - https://moodle.org/
+//
+// Moodle is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// Moodle is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with Moodle.  If not, see <https://www.gnu.org/licenses/>.
+
+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 @@
+<?php
+defined('MOODLE_INTERNAL') || die(); // Make this always the 1st line in all CS fixtures.
+
+class correct_setup_teardown_test extends Something {
+    public function setUp(): void {
+        // Call parent.
+        parent::setUp();
+    }
+
+    public function tearDown(): void {
+        parent::tearDown();
+        // Parent called.
+    }
+
+    public static function setUpBeforeClass(): void {
+        // Call parent.
+        parent::setUpBeforeClass();
+    }
+
+    public static function tearDownAfterClass(): void {
+        parent::tearDownAfterClass();
+        // Parent called.
+    }
+}
+
+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();
+     }
+}
+
+class yet_another_correct_setup_teardown_test extends Something {
+    public function setUp(): void {
+        global $CFG;
+        parent::someThing();
+        parent::setUp();
+    }
+    public function tearDown(): void {
+        parent::setup(); // No case matching, aka, valid method to call.
+        parent::tearDown();
+    }
+    public static function setUpBeforeClass(): void {
+        parent::anotherThing();
+        parent::setUpBeforeClass();
+    }
+    public static function tearDownAfterClass(): void {
+        parent::tearDownAfterClass();
+        parent::yetAnotherThing();
+    }
+}
+
+class not_test_hence_ignored extends Something {
+    public function setUp(): void {
+    }
+
+    public function tearDown(): void {
+    }
+
+    public static function setUpBeforeClass(): void {
+    }
+
+    public static function tearDownAfterClass(): void {
+    }
+}
diff --git a/moodle/Tests/Sniffs/PHPUnit/fixtures/ParentSetUpTearDownProblems.php b/moodle/Tests/Sniffs/PHPUnit/fixtures/ParentSetUpTearDownProblems.php
new file mode 100644
index 00000000..c4a5dc6a
--- /dev/null
+++ b/moodle/Tests/Sniffs/PHPUnit/fixtures/ParentSetUpTearDownProblems.php
@@ -0,0 +1,89 @@
+<?php
+defined('MOODLE_INTERNAL') || die(); // Make this always the 1st line in all CS fixtures.
+
+class missing_setup_teardown_test extends Something {
+    public function setUp(): void {
+    }
+
+    public function tearDown(): void {
+    }
+
+    public static function setUpBeforeClass(): void {
+    }
+
+    public static function tearDownAfterClass(): void {
+    }
+}
+
+class duplicate_and_incorrct_setup_teardown_test extends Something {
+    public function setUp(): void {
+        parent::setUp();
+        parent::setUp();
+        parent::tearDown(); // This is wrong.
+        parent::anotherMethodOk();
+    }
+    public function tearDown(): void {
+        parent::tearDown();
+        parent::tearDown();
+        parent::anotherMethodOk();
+        parent::setUp(); // This is wrong.
+    }
+    public static function setUpBeforeClass(): void {
+        parent::tearDownAfterClass(); // This is wrong.
+        parent::setUpBeforeClass();
+        parent::anotherMethodOk();
+        parent::setUpBeforeClass();
+    }
+    public static function tearDownAfterClass(): void {
+        parent::anotherMethodOk();
+        parent::tearDownAfterClass();
+        parent::tearDownAfterClass();
+        parent::setUpBeforeClass(); // This is wrong.
+    }
+}
+
+class best_insertion_setup_teardown_test extends Something {
+    public function setUp(): void {
+
+        // This is a comment.
+
+        global $CFG, $CFG2,
+               $CFG3, CFG4;
+
+        require(
+            'somefile.php'
+        );
+
+        // Another comment.
+        global $USER;
+
+        include_once('somefile.php');
+    }
+    public function tearDown(): void {
+        // This is a comment.
+
+        global $CFG;
+
+        require('somefile.php');
+    }
+    public static function setUpBeforeClass(): void {
+        $a = 3;
+
+        // This is a comment.
+
+        global $CFG;
+
+        require('somefile.php');
+
+        // Another comment.
+        global $USER;
+
+        include('somefile.php');
+    }
+    public static function tearDownAfterClass(): void {
+        $a = 3:
+        // This is a comment.
+        global $CFG;
+        require('somefile.php');
+    }
+}
diff --git a/moodle/Tests/Sniffs/PHPUnit/fixtures/ParentSetUpTearDownProblems.php.fixed b/moodle/Tests/Sniffs/PHPUnit/fixtures/ParentSetUpTearDownProblems.php.fixed
new file mode 100644
index 00000000..22726ded
--- /dev/null
+++ b/moodle/Tests/Sniffs/PHPUnit/fixtures/ParentSetUpTearDownProblems.php.fixed
@@ -0,0 +1,97 @@
+<?php
+defined('MOODLE_INTERNAL') || die(); // Make this always the 1st line in all CS fixtures.
+
+class missing_setup_teardown_test extends Something {
+    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();
+    }
+}
+
+class duplicate_and_incorrct_setup_teardown_test extends Something {
+    public function setUp(): void {
+        parent::setUp();
+        parent::setUp();
+        parent::tearDown(); // This is wrong.
+        parent::anotherMethodOk();
+    }
+    public function tearDown(): void {
+        parent::tearDown();
+        parent::tearDown();
+        parent::anotherMethodOk();
+        parent::setUp(); // This is wrong.
+    }
+    public static function setUpBeforeClass(): void {
+        parent::tearDownAfterClass(); // This is wrong.
+        parent::setUpBeforeClass();
+        parent::anotherMethodOk();
+        parent::setUpBeforeClass();
+    }
+    public static function tearDownAfterClass(): void {
+        parent::anotherMethodOk();
+        parent::tearDownAfterClass();
+        parent::tearDownAfterClass();
+        parent::setUpBeforeClass(); // This is wrong.
+    }
+}
+
+class best_insertion_setup_teardown_test extends Something {
+    public function setUp(): void {
+
+        // This is a comment.
+
+        global $CFG, $CFG2,
+               $CFG3, CFG4;
+
+        require(
+            'somefile.php'
+        );
+
+        // Another comment.
+        global $USER;
+
+        include_once('somefile.php');
+        parent::setUp();
+    }
+    public function tearDown(): void {
+        // This is a comment.
+
+        global $CFG;
+
+        require('somefile.php');
+        parent::tearDown();
+    }
+    public static function setUpBeforeClass(): void {
+        parent::setUpBeforeClass();
+        $a = 3;
+
+        // This is a comment.
+
+        global $CFG;
+
+        require('somefile.php');
+
+        // Another comment.
+        global $USER;
+
+        include('somefile.php');
+    }
+    public static function tearDownAfterClass(): void {
+        $a = 3:
+        // This is a comment.
+        global $CFG;
+        require('somefile.php');
+        parent::tearDownAfterClass();
+    }
+}