UtilityMethodTestCase: safeguard against duplicate test markers

Sister-PR to upstream PHPCSStandards/PHP_CodeSniffer 777 addressing issue PHPCSStandards/PHP_CodeSniffer 773.

This commit adds a `testTestMarkersAreUnique()` test method to the `UtilityMethodTestCase` class to automatically verify that the case file in use by the child test class only contains unique test markers.

The actual logic for the test is in a custom, `static`, assertion `assertTestMarkersAreUnique()` to allow for calling the assertion directly if an additional test case file is tokenized for the test.

Includes unit tests.
Includes updating a few tests related to the `UtilityMethodTestCase` itself to not run the `testTestMarkersAreUnique()` test when the test is testing an error condition/doesn't have a valid `$phpcsFile` property set.

Author: jrfnl

PHPCSUtils/TestUtils/UtilityMethodTestCase.php

@@ -378,6 +378,57 @@ public static function usesPhp8NameTokens()
         return \version_compare(Helper::getVersion(), '3.99.99', '>=');
     }
 
+    /**
+     * Test QA: verify that a test case file does not contain any duplicate test markers.
+     *
+     * When a test case file contains a lot of test cases, it is easy to overlook that a test marker name
+     * is already in use.
+     * A test wouldn't necessarily fail on this, but would not be testing what is intended to be tested as
+     * it would be verifying token properties for the wrong token.
+     *
+     * This test safeguards against this.
+     *
+     * @since 1.1.0
+     *
+     * @coversNothing
+     *
+     * @return void
+     */
+    public function testTestMarkersAreUnique()
+    {
+        $this->assertTestMarkersAreUnique(self::$phpcsFile);
+    }
+
+    /**
+     * Assertion to verify that a test case file does not contain any duplicate test markers.
+     *
+     * @since 1.1.0
+     *
+     * @param \PHP_CodeSniffer\Files\File $phpcsFile The file to validate.
+     *
+     * @return void
+     */
+    public static function assertTestMarkersAreUnique(File $phpcsFile)
+    {
+        $tokens = $phpcsFile->getTokens();
+
+        // Collect all marker comments in the file.
+        $seenComments = [];
+        for ($i = 0; $i < $phpcsFile->numTokens; $i++) {
+            if ($tokens[$i]['code'] !== \T_COMMENT) {
+                continue;
+            }
+
+            if (\stripos($tokens[$i]['content'], '/* test') !== 0) {
+                continue;
+            }
+
+            $seenComments[] = $tokens[$i]['content'];
+        }
+
+        self::assertSame(\array_unique($seenComments), $seenComments, 'Duplicate test markers found.');
+    }
+
     /**
      * Get the token pointer for a target token based on a specific comment.
      *

Tests/TestUtils/UtilityMethodTestCase/ExpectPhpcsExceptionTest.php

@@ -60,6 +60,19 @@ public static function resetTestFile()
         // Deliberately left empty.
     }
 
+    /**
+     * Overload the "normal" test marker QA check - this test class does not have a File object.
+     *
+     * @coversNothing
+     * @doesNotPerformAssertions
+     *
+     * @return void
+     */
+    public function testTestMarkersAreUnique()
+    {
+        // Deliberately left empty.
+    }
+
     /**
      * Test that the helper method to handle cross-version testing of exceptions in PHPUnit
      * works correctly.

Tests/TestUtils/UtilityMethodTestCase/FailedToTokenizeTest.php

@@ -35,6 +35,19 @@ public static function setUpTestFile()
         // Deliberately left empty.
     }
 
+    /**
+     * Overload the "normal" test marker QA check - this test class does not have a valid File object.
+     *
+     * @coversNothing
+     * @doesNotPerformAssertions
+     *
+     * @return void
+     */
+    public function testTestMarkersAreUnique()
+    {
+        // Deliberately left empty.
+    }
+
     /**
      * Test that the setUpTestFile() fails a test when the tokenizer errored out.
      *

Tests/TestUtils/UtilityMethodTestCase/GetTargetTokenFileNotFoundTest.php

@@ -34,6 +34,19 @@ public static function setUpTestFile()
         // Deliberately left empty.
     }
 
+    /**
+     * Overload the "normal" test marker QA check - this test class does not have a File object.
+     *
+     * @coversNothing
+     * @doesNotPerformAssertions
+     *
+     * @return void
+     */
+    public function testTestMarkersAreUnique()
+    {
+        // Deliberately left empty.
+    }
+
     /**
      * Test the behaviour of the getTargetToken() method when the test case file has not been tokenized.
      *

Tests/TestUtils/UtilityMethodTestCase/MissingCaseFileTest.php

@@ -35,6 +35,19 @@ public static function setUpTestFile()
         // Deliberately left empty.
     }
 
+    /**
+     * Overload the "normal" test marker QA check - this test class does not have a File object.
+     *
+     * @coversNothing
+     * @doesNotPerformAssertions
+     *
+     * @return void
+     */
+    public function testTestMarkersAreUnique()
+    {
+        // Deliberately left empty.
+    }
+
     /**
      * Test that the setUpTestFile() fails a test when the test case file is missing.
      *

Tests/TestUtils/UtilityMethodTestCase/ResetTestFileTest.php

@@ -28,6 +28,10 @@ final class ResetTestFileTest extends PolyfilledTestCase
     /**
      * Overload the "normal" set up as it needs to be run from within the actual test(s) to ensure we have a valid test.
      *
+     * Note: We don't rely on this method being called "before class" as the tests in this class reset the statics on
+     * the UtilityMethodTestCase, so we need to be sure the static $caseFile property is set (again) before parsing
+     * a file and do that by calling this method directly from within each of the tests.
+     *
      * @beforeClass
      *
      * @return void
@@ -38,6 +42,20 @@ public static function setUpTestFile()
         // Deliberately not running the actual setUpTestFile() method.
     }
 
+    /**
+     * Overload the "normal" test marker QA check - this test class resets the property containing the File object,
+     * so there will be no valid File + the case file is already validated in the `SetUpTestFileTest` class anyway.
+     *
+     * @coversNothing
+     * @doesNotPerformAssertions
+     *
+     * @return void
+     */
+    public function testTestMarkersAreUnique()
+    {
+        // Deliberately left empty.
+    }
+
     /**
      * Test that the static class properties in the class are correctly reset.
      *
@@ -48,6 +66,7 @@ public function testTearDownCleansUpStaticTestCaseClassProperties()
         // Initialize a test, which should change the values of most static properties.
         self::$tabWidth      = 2;
         self::$selectedSniff = ['Test.Test.Test'];
+        self::setUpTestFile();
         parent::setUpTestFile();
 
         // Verify that (most) properties no longer have their original value.

Tests/TestUtils/UtilityMethodTestCase/SetUpTestFileTest.php

@@ -37,6 +37,22 @@ public static function setUpTestFile()
         // Deliberately not running the actual setUpTestFile() method.
     }
 
+    /**
+     * Overload the "normal" test marker QA check - this test class does not have a File object in the $phpcsFile property.
+     *
+     * @coversNothing
+     *
+     * @return void
+     */
+    public function testTestMarkersAreUnique()
+    {
+        parent::setUpTestFile();
+
+        $this->assertTestMarkersAreUnique(self::$phpcsFile);
+
+        parent::resetTestFile();
+    }
+
     /**
      * Test that the setUpTestFile() method works correctly.
      *

Tests/TestUtils/UtilityMethodTestCase/TestMarkersAreUniqueFailsTest.inc

@@ -0,0 +1,13 @@
+<?php
+
+/* testMarkerA */
+echo 'hello';
+
+/* testMarkerB */
+echo 'hello';
+
+/* testMarkerA */
+echo 'hello';
+
+/* testMarkerB */
+echo 'hello';

Tests/TestUtils/UtilityMethodTestCase/TestMarkersAreUniqueFailsTest.php

@@ -0,0 +1,45 @@
+<?php
+/**
+ * PHPCSUtils, utility functions and classes for PHP_CodeSniffer sniff developers.
+ *
+ * @package   PHPCSUtils
+ * @copyright 2019-2020 PHPCSUtils Contributors
+ * @license   https://opensource.org/licenses/LGPL-3.0 LGPL3
+ * @link      https://github.com/PHPCSStandards/PHPCSUtils
+ */
+
+namespace PHPCSUtils\Tests\TestUtils\UtilityMethodTestCase;
+
+use PHPCSUtils\Tests\PolyfilledTestCase;
+
+/**
+ * Tests for the \PHPCSUtils\TestUtils\UtilityMethodTestCase class.
+ *
+ * @covers \PHPCSUtils\TestUtils\UtilityMethodTestCase::testTestMarkersAreUnique
+ * @covers \PHPCSUtils\TestUtils\UtilityMethodTestCase::assertTestMarkersAreUnique
+ *
+ * @since 1.1.0
+ */
+final class TestMarkersAreUniqueFailsTest extends PolyfilledTestCase
+{
+
+    /**
+     * Overload the "normal" test marker QA check - this test class does not have a valid File object.
+     *
+     * @return void
+     */
+    public function testTestMarkersAreUnique()
+    {
+        $msg       = "Duplicate test markers found.\nFailed asserting that ";
+        $exception = 'PHPUnit\Framework\AssertionFailedError';
+        if (\class_exists('PHPUnit_Framework_AssertionFailedError')) {
+            // PHPUnit < 6.
+            $exception = 'PHPUnit_Framework_AssertionFailedError';
+        }
+
+        $this->expectException($exception);
+        $this->expectExceptionMessage($msg);
+
+        parent::testTestMarkersAreUnique();
+    }
+}

Tests/TestUtils/UtilityMethodTestCase/TestMarkersAreUniqueNoMarkersTest.inc

@@ -0,0 +1,4 @@
+<?php
+
+echo 'hello';
+echo 'hello';