Merge pull request #642 from PHPCSStandards/feature/utilitymethodtest…

…case-add-safeguard-against-duplicate-markers

UtilityMethodTestCase: safeguard against duplicate test markers

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/BackCompat/BCFile/GetMethodParametersTest.inc

@@ -118,7 +118,7 @@ function messyDeclaration(
     ?\MyNS /* comment */
         \ SubCat // phpcs:ignore Standard.Cat.Sniff -- for reasons.
             \  MyClass $a,
-    $b /* test */ = /* test */ 'default' /* test*/,
+    $b /* comment */ = /* comment */ 'default' /* comment*/,
     // phpcs:ignore Stnd.Cat.Sniff -- For reasons.
     ? /*comment*/
         bool // phpcs:disable Stnd.Cat.Sniff -- For reasons.

Tests/BackCompat/BCFile/GetMethodParametersTest.php

@@ -1226,8 +1226,8 @@ public function testMessyDeclaration()
         $expected[1] = [
             'token'               => ($php8Names === true) ? 28 : 29,
             'name'                => '$b',
-            'content'             => "\$b /* test */ = /* test */ 'default' /* test*/",
-            'default'             => "'default' /* test*/",
+            'content'             => "\$b /* comment */ = /* comment */ 'default' /* comment*/",
+            'default'             => "'default' /* comment*/",
             'default_token'       => ($php8Names === true) ? 36 : 37,
             'default_equal_token' => ($php8Names === true) ? 32 : 33,
             'has_attributes'      => false,

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';

Tests/TestUtils/UtilityMethodTestCase/TestMarkersAreUniqueNoMarkersTest.php

@@ -0,0 +1,35 @@
+<?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 TestMarkersAreUniqueNoMarkersTest extends PolyfilledTestCase
+{
+
+    /**
+     * Overload the "normal" test marker QA check, but only to overload the `@covers` tags.
+     *
+     * @return void
+     */
+    public function testTestMarkersAreUnique()
+    {
+        parent::testTestMarkersAreUnique();
+    }
+}

Tests/TestUtils/UtilityMethodTestCase/TestMarkersAreUniqueTest.inc

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

Tests/TestUtils/UtilityMethodTestCase/TestMarkersAreUniqueTest.php

@@ -0,0 +1,35 @@
+<?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 TestMarkersAreUniqueTest extends PolyfilledTestCase
+{
+
+    /**
+     * Overload the "normal" test marker QA check, but only to overload the `@covers` tags.
+     *
+     * @return void
+     */
+    public function testTestMarkersAreUnique()
+    {
+        parent::testTestMarkersAreUnique();
+    }
+}