TestUtils/ConfigDouble: bug fix - always reset Config statics after use

jrfnl · jrfnl · commit acd940a0f34f · 2024-06-09T14:33:12.000+02:00
The PHPCS native `Config` class uses a number of static properties, which may be updated during tests. These were previously reset to their default values in the `UtilityMethodTestCase::resetTestFile()` method, but this reset was inadvertently removed in commit 4f0f9a4 with the reasoning that, if all tests use the `ConfigDouble`, the reset would no longer be needed as the `ConfigDouble` resets on being initialized. The flaw in this logic is that not all tests are guaranteed to use the `ConfigDouble`, which means that without the reset, the `Config` class may be left "dirty" after tests using the `ConfigDouble`, which could break tests. This commit fixes this issue by: * Adding a `__destruct()` method to the `ConfigDouble` class which will reset the static properties on the PHPCS native `Config` class whenever an object created from this class is destroyed. * Explicitly calling the `__destruct()` method from the `UtilityMethodTestCase::resetTestFile()` method to ensure it is always run after a test has finished ("after class"), even if there would still be a lingering reference to the object. Includes tests for the new `__destruct()` method. Includes an additional test for the `UtilityMethodTestCase` class to ensure this snafu cannot come back without tests failing on it.
diff --git a/PHPCSUtils/TestUtils/ConfigDouble.php b/PHPCSUtils/TestUtils/ConfigDouble.php
@@ -90,6 +90,22 @@ public function __construct(array $cliArgs = [], $skipSettingStandard = false, $
         }
     }
 
+    /**
+     * Ensures the static properties in the Config class are reset to their default values
+     * when the ConfigDouble is no longer used.
+     *
+     * @since 1.1.0
+     *
+     * @return void
+     */
+    public function __destruct()
+    {
+        $this->setStaticConfigProperty('overriddenDefaults', []);
+        $this->setStaticConfigProperty('executablePaths', []);
+        $this->setStaticConfigProperty('configData', null);
+        $this->setStaticConfigProperty('configDataFile', null);
+    }
+
     /**
      * Sets the command line values and optionally prevents a file system search for a custom ruleset.
      *
diff --git a/PHPCSUtils/TestUtils/UtilityMethodTestCase.php b/PHPCSUtils/TestUtils/UtilityMethodTestCase.php
@@ -310,6 +310,16 @@ public function skipJSCSSTestsOnPHPCS4()
      */
     public static function resetTestFile()
     {
+        /*
+         * Explicitly trigger __destruct() on the ConfigDouble to reset the Config statics.
+         * The explicit method call prevents potential stray test-local references to the $config object
+         * preventing the destructor from running the clean up (which without stray references would be
+         * automagically triggered when `self::$phpcsFile` is reset, but we can't definitively rely on that).
+         */
+        if (isset(self::$phpcsFile)) {
+            self::$phpcsFile->config->__destruct();
+        }
+
         self::$phpcsVersion  = '0';
         self::$fileExtension = 'inc';
         self::$caseFile      = '';
diff --git a/Tests/TestUtils/ConfigDouble/ConfigDoubleTest.php b/Tests/TestUtils/ConfigDouble/ConfigDoubleTest.php
@@ -10,7 +10,9 @@
 
 namespace PHPCSUtils\Tests\TestUtils\ConfigDouble;
 
+use PHPCSUtils\BackCompat\Helper;
 use PHPCSUtils\TestUtils\ConfigDouble;
+use ReflectionProperty;
 use Yoast\PHPUnitPolyfills\TestCases\TestCase;
 
 /**
@@ -25,6 +27,23 @@
 final class ConfigDoubleTest extends TestCase
 {
 
+    /**
+     * Reset the static properties in the Config class to their true defaults to prevent this class
+     * from influencing other tests.
+     *
+     * @afterClass
+     *
+     * @return void
+     */
+    public static function resetConfigToDefaults()
+    {
+        self::setStaticConfigProperty('overriddenDefaults', []);
+        self::setStaticConfigProperty('executablePaths', []);
+        self::setStaticConfigProperty('configData', null);
+        self::setStaticConfigProperty('configDataFile', null);
+        $_SERVER['argv'] = [];
+    }
+
     /**
      * Verify that the static properties in the Config class get cleared between instances.
      *
@@ -200,4 +219,119 @@ public function testReportWidthOverrideIsSkippedOnRequest()
             $this->assertNotSame(80, $config->reportWidth, 'Report width has still been set to 80');
         }
     }
+
+    /**
+     * Verify that the static properties in the Config class are reset when the object is destroyed.
+     *
+     * @covers ::__destruct
+     *
+     * @return void
+     */
+    public function testDestruct()
+    {
+        $standard     = 'Squiz';
+        $reportWidth  = 1250;
+        $fakeConfFile = 'path/to/file.conf';
+        $toolName     = 'a_tool';
+
+        // Create the ConfigDouble object and change the value of a few static properties to allow for testing the reset.
+        $cliArgs = [
+            '--standard=' . $standard,
+            '--report-width=' . $reportWidth,
+            '--runtime-set',
+            'arbitraryKey',
+            'arbitraryValue',
+        ];
+        $config  = new ConfigDouble($cliArgs);
+
+        $config->setStaticConfigProperty('configDataFile', $fakeConfFile);
+        $config::getExecutablePath($toolName);
+
+        // Verify the static properties in the Config are set to something other than their default value.
+        $this->assertSame([$standard], $config->standards, 'Precondition check: Standards was not set to Squiz');
+        $this->assertSame($reportWidth, $config->reportWidth, 'Precondition check: Report width was not set to 1250');
+        $this->assertSame(
+            'arbitraryValue',
+            Helper::getConfigData('arbitraryKey'),
+            'Precondition check: ArbitraryKey property was not set on the Config class'
+        );
+
+        $overriddenDefaults = $this->getStaticConfigProperty('overriddenDefaults', $config);
+        $this->assertIsArray($overriddenDefaults, 'Precondition check: overriddenDefaults property is not an array');
+        $this->assertNotEmpty($overriddenDefaults, 'Precondition check: overriddenDefaults property is an empty array');
+
+        $this->assertSame(
+            [$toolName => null],
+            $this->getStaticConfigProperty('executablePaths', $config),
+            'Precondition check: executablePaths is still an empty array'
+        );
+
+        $configData = $this->getStaticConfigProperty('configData', $config);
+        $this->assertIsArray($configData, 'Precondition check: configData property is not an array');
+        $this->assertNotEmpty($configData, 'Precondition check: configData property is an empty array');
+
+        $this->assertSame(
+            $fakeConfFile,
+            $this->getStaticConfigProperty('configDataFile', $config),
+            'Precondition check: configDataFile property has not been set'
+        );
+
+        // Destroy the object.
+        unset($config);
+
+        // Verify the static properties in the Config are reset to their default values.
+        $this->assertSame([], $this->getStaticConfigProperty('overriddenDefaults'), 'overriddenDefaults reset failed');
+        $this->assertSame([], $this->getStaticConfigProperty('executablePaths'), 'executablePaths reset failed');
+        $this->assertNull($this->getStaticConfigProperty('configData'), 'configData reset failed');
+        $this->assertNull($this->getStaticConfigProperty('configDataFile'), 'configDataFile reset failed');
+
+        // This assertion must be last as it re-initializes the $configData and $configDataFile properties.
+        $this->assertNull(Helper::getConfigData('arbitraryKey'), 'arbitraryKey property is still set');
+    }
+
+    /**
+     * Helper function to retrieve the value of a private static property on the Config class.
+     *
+     * @param string                  $name   The name of the property to retrieve.
+     * @param \PHP_CodeSniffer\Config $config Optional. The config object.
+     *
+     * @return mixed
+     */
+    private function getStaticConfigProperty($name, $config = null)
+    {
+        $property = new ReflectionProperty('PHP_CodeSniffer\Config', $name);
+        $property->setAccessible(true);
+
+        if ($name === 'overriddenDefaults' && \version_compare(Helper::getVersion(), '3.99.99', '>')) {
+            // The `overriddenDefaults` property is no longer static on PHPCS 4.0+.
+            if (isset($config)) {
+                return $property->getValue($config);
+            } else {
+                return [];
+            }
+        }
+
+        return $property->getValue();
+    }
+
+    /**
+     * Helper function to set the value of a private static property on the Config class.
+     *
+     * @param string $name  The name of the property to set.
+     * @param mixed  $value The value to set the property to.
+     *
+     * @return void
+     */
+    public static function setStaticConfigProperty($name, $value)
+    {
+        $property = new ReflectionProperty('PHP_CodeSniffer\Config', $name);
+        $property->setAccessible(true);
+
+        // The `overriddenDefaults` property is no longer static on PHPCS 4.0+, so ignore it.
+        if ($name !== 'overriddenDefaults' && \version_compare(Helper::getVersion(), '3.99.99', '<=')) {
+            $property->setValue(null, $value);
+        }
+
+        $property->setAccessible(false);
+    }
 }
diff --git a/Tests/TestUtils/UtilityMethodTestCase/ResetTestFileTest.php b/Tests/TestUtils/UtilityMethodTestCase/ResetTestFileTest.php
@@ -10,7 +10,10 @@
 
 namespace PHPCSUtils\Tests\TestUtils\UtilityMethodTestCase;
 
+use PHP_CodeSniffer\Config;
+use PHPCSUtils\BackCompat\Helper;
 use PHPCSUtils\Tests\PolyfilledTestCase;
+use ReflectionProperty;
 
 /**
  * Tests for the \PHPCSUtils\TestUtils\UtilityMethodTestCase class.
@@ -40,7 +43,7 @@ public static function setUpTestFile()
      *
      * @return void
      */
-    public function testTearDown()
+    public function testTearDownCleansUpStaticTestCaseClassProperties()
     {
         // Initialize a test, which should change the values of most static properties.
         self::$tabWidth      = 2;
@@ -66,4 +69,114 @@ public function testTearDown()
         $this->assertNull(self::$phpcsFile, 'phpcsFile was not reset');
         $this->assertSame(['Dummy.Dummy.Dummy'], self::$selectedSniff, 'selectedSniff was not reset');
     }
+
+    /**
+     * Test that the static properties in the Config class are correctly reset.
+     *
+     * Ensure that Config set for one test does not influence the next test.
+     *
+     * @return void
+     */
+    public function testTearDownCleansUpStaticConfigProperties()
+    {
+        $fakeConfFile = 'path/to/file.conf';
+        $toolName     = 'a_tool';
+
+        // Set up preconditions.
+        self::setUpTestFile();
+        parent::setUpTestFile();
+
+        $config = self::$phpcsFile->config;
+        Helper::setConfigData('arbitraryKey', 'arbitraryValue', true, $config);
+        $config->setStaticConfigProperty('configDataFile', $fakeConfFile);
+        $config::getExecutablePath($toolName);
+
+        // Verify the static properties in the Config are set to something other than their default value.
+        $this->assertSame(['PSR1'], $config->standards, 'Precondition check: Standards was not set to PSR1');
+
+        $overriddenDefaults = $this->getStaticConfigProperty('overriddenDefaults', $config);
+        $this->assertIsArray($overriddenDefaults, 'Precondition check: overriddenDefaults property is not an array');
+        $this->assertNotEmpty($overriddenDefaults, 'Precondition check: overriddenDefaults property is an empty array');
+
+        $this->assertSame(
+            [$toolName => null],
+            $this->getStaticConfigProperty('executablePaths', $config),
+            'Precondition check: executablePaths is still an empty array'
+        );
+
+        $configData = $this->getStaticConfigProperty('configData', $config);
+        $this->assertIsArray($configData, 'Precondition check: configData property is not an array');
+        $this->assertNotEmpty($configData, 'Precondition check: configData property is an empty array');
+
+        $this->assertSame(
+            $fakeConfFile,
+            $this->getStaticConfigProperty('configDataFile', $config),
+            'Precondition check: configDataFile property has not been set'
+        );
+
+        // Reset the file as per the "afterClass"/tear down method.
+        parent::resetTestFile();
+
+        // Verify that the reset also reset the static properties on the Config class.
+        $this->assertSame(
+            [],
+            $this->getStaticConfigProperty('overriddenDefaults'),
+            'overriddenDefaults reset failed'
+        );
+        $this->assertSame([], $this->getStaticConfigProperty('executablePaths'), 'executablePaths reset failed');
+        $this->assertNull($this->getStaticConfigProperty('configData'), 'configData reset failed');
+        $this->assertNull($this->getStaticConfigProperty('configDataFile'), 'configDataFile reset failed');
+
+        $this->assertNull(Helper::getConfigData('arbitraryKey'), 'arbitraryKey property is still set');
+
+        // Now check that if a new Config is created for another test, that previously overridden defaults can be set again.
+        $newConfig = new Config(['--standard=Squiz']);
+
+        // Verify the new Config does not have leaked property values from the Config from the "previous test".
+        $this->assertSame(['Squiz'], $newConfig->standards, 'New standards choice was not set to Squiz');
+        $this->assertSame(
+            ['standards' => true],
+            $this->getStaticConfigProperty('overriddenDefaults', $newConfig),
+            'overriddenDefaults has not been reinitialized'
+        );
+
+        // Verify that previously overridden config property can be written to.
+        $newValue = 'new value';
+        $this->assertTrue(
+            Helper::setConfigData('arbitraryKey', $newValue, true, $newConfig),
+            'New value for arbitraryKey is not accepted'
+        );
+        $this->assertSame(
+            $newValue,
+            Helper::getConfigData('arbitraryKey'),
+            'Previously overridden default was not allowed to be set'
+        );
+    }
+
+    /**
+     * Helper function to retrieve the value of a private static property on the Config class.
+     *
+     * @param string                  $name   The name of the property to retrieve.
+     * @param \PHP_CodeSniffer\Config $config Optional. The config object.
+     *
+     * @return mixed
+     */
+    private function getStaticConfigProperty($name, $config = null)
+    {
+        $property = new ReflectionProperty('PHP_CodeSniffer\Config', $name);
+        $property->setAccessible(true);
+
+        if ($name === 'overriddenDefaults'
+            && (self::$phpcsVersion === '0' || \version_compare(self::$phpcsVersion, '3.99.99', '>'))
+        ) {
+            // The `overriddenDefaults` property is no longer static on PHPCS 4.0+.
+            if (isset($config)) {
+                return $property->getValue($config);
+            } else {
+                return [];
+            }
+        }
+
+        return $property->getValue();
+    }
 }
diff --git a/phpstan.neon.dist b/phpstan.neon.dist
@@ -35,6 +35,17 @@ parameters:
             count: 1
 
         # Level 2
+        # The __destruct() method exists on the ConfigDouble, not the PHPCS native Config. This is 100% okay.
+        -
+            message: '`^Call to an undefined method PHP_CodeSniffer\\Config::__destruct\(\)\.$`'
+            path: PHPCSUtils\TestUtils\UtilityMethodTestCase.php
+            count: 1
+        # The setStaticConfigProperty() method exists on the ConfigDouble, not the PHPCS native Config. This is 100% okay.
+        -
+            message: '`^Call to an undefined method PHP_CodeSniffer\\Config::setStaticConfigProperty\(\)\.$`'
+            path: Tests\TestUtils\UtilityMethodTestCase\ResetTestFileTest.php
+            count: 1
+
         # Ignoring as this refers to a non-mocked method on the original class. This is 100% okay.
         -
             message: '`^Call to an undefined method PHPUnit\\Framework\\MockObject\\MockObject::process\(\)\.$`'
