From 5004b32241bb7ffa89a0afd63c5ecf9dd11213e7 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 20 May 2023 07:30:08 +0200 Subject: [PATCH 1/2] Config: only determine screen width if/when needed Follow up on 3761 for which tests were added in 3820. I'm not sure about *nix, but on Windows, the call to `shell_exec('stty ...')` is slow. As things were, the call to `shell_exec('stty ...')` would be: * Made when the initial default value for `reportWidth` is being set in `restoreDefaults()`. * Potentially made a second time if the users `CodeSniffer.conf` file contained a `report_width => 'auto'` entry. * Potentially made a third time if any of the rulesets used for the run contained a `` entry. * Potentially made a fourth time if `--report-width=auto` would be passed from the command-line. This is inefficient for the following reasons: 1. The output from `shell_exec('stty ...')` does not change between calls (well, providing the user doesn't resize their terminal in the microseconds between calls) 2. We don't actually need to _know_ the value `'auto'` translates to, until the `reportWidth` will be _used_. Taking the above into account, making the same call up to four times is not desirable. This commit moves the translation from `'auto'` to an actual terminal width from the `__set()` method to the `__get()` method and overwrites the `reportWidth` value from `'auto'` with the actual terminal width value, if available, and with the default value if the terminal width could not be determined. This means that subsequent calls to `__get()` for the `reportWidth` will automatically use the previously determined value instead of trying to determine value again. This removes the inefficiency and should make PHPCS runs a little bit faster (at the very least on Windows). The only time multiple calls to `shell_exec('stty...')` could now need to be made, would be if the `reportWidth` would be changed (back to `'auto'`) between the first retrieval and a subsequent retrieval of the `reportWidth` value. As things are, this will never happen in a normal PHPCS run, though could possibly happen in a test situation. --- src/Config.php | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/src/Config.php b/src/Config.php index 1ae4c77ee3..d4210c74e3 100644 --- a/src/Config.php +++ b/src/Config.php @@ -207,6 +207,22 @@ public function __get($name) throw new RuntimeException("ERROR: unable to get value of property \"$name\""); } + // Figure out what the terminal width needs to be for "auto". + if ($name === 'reportWidth' && $this->settings[$name] === 'auto') { + if (function_exists('shell_exec') === true) { + $dimensions = shell_exec('stty size 2>&1'); + if (is_string($dimensions) === true && preg_match('|\d+ (\d+)|', $dimensions, $matches) === 1) { + $this->settings[$name] = (int) $matches[1]; + } + } + + if ($this->settings[$name] === 'auto') { + // If shell_exec wasn't available or didn't yield a usable value, set to the default. + // This will prevent subsequent retrievals of the reportWidth from making another call to stty. + $this->settings[$name] = self::DEFAULT_REPORT_WIDTH; + } + } + return $this->settings[$name]; }//end __get() @@ -229,13 +245,9 @@ public function __set($name, $value) switch ($name) { case 'reportWidth' : - // Support auto terminal width. - if ($value === 'auto' && function_exists('shell_exec') === true) { - $dimensions = shell_exec('stty size 2>&1'); - if (is_string($dimensions) === true && preg_match('|\d+ (\d+)|', $dimensions, $matches) === 1) { - $value = (int) $matches[1]; - break; - } + if (is_string($value) === true && $value === 'auto') { + // Nothing to do. Leave at 'auto'. + break; } if (is_int($value) === true) { @@ -246,6 +258,7 @@ public function __set($name, $value) $value = self::DEFAULT_REPORT_WIDTH; } break; + case 'standards' : $cleaned = []; From 020fbfa75e594383e4a2449d6b9ffb1a24d1582d Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 20 May 2023 07:39:02 +0200 Subject: [PATCH 2/2] AbstractMethodUnitTest: take advantage of the change in reportWidth handling For the tests using the `AbstractMethodUnitTest` class, the `reportWidth` and most other config settings are irrelevant. This commit changes some of the set up/tear down for the test to make the use of the `Config` class more efficient. This should make the tests using the `AbstractMethodUnitTest` class as their base significantly faster (at the very least on Windows). While not benchmarked properly, I have done some comparisons with the test runs on my local machine on Windows. * `phpunit --filter Core` (= the tests which use this base class + a few extra tests): Before: **2 minutes**. After: **8 seconds**. * The same effect can be seen when running `phpunit` without a `--filter`: Before: **7 minutes**. After: **5 minutes**. * And when I apply a similar change to the one made here to the base test class in PHPCSUtils (4621 tests): Before: **2.5 minutes**. After: **1 second**. --- tests/Core/AbstractMethodUnitTest.php | 42 +++++++++++++++++++++++++-- 1 file changed, 40 insertions(+), 2 deletions(-) diff --git a/tests/Core/AbstractMethodUnitTest.php b/tests/Core/AbstractMethodUnitTest.php index 4d4f546995..50dd63ac69 100644 --- a/tests/Core/AbstractMethodUnitTest.php +++ b/tests/Core/AbstractMethodUnitTest.php @@ -13,6 +13,7 @@ use PHP_CodeSniffer\Ruleset; use PHP_CodeSniffer\Files\DummyFile; use PHPUnit\Framework\TestCase; +use ReflectionProperty; abstract class AbstractMethodUnitTest extends TestCase { @@ -45,9 +46,22 @@ abstract class AbstractMethodUnitTest extends TestCase */ public static function setUpBeforeClass() { - $config = new Config(); - $config->standards = ['PSR1']; + /* + * Set the static properties in the Config class to specific values for performance + * and to clear out values from other tests. + */ + self::setStaticConfigProperty('executablePaths', []); + + // Set to a usable value to circumvent Config trying to find a phpcs.xml config file. + self::setStaticConfigProperty('overriddenDefaults', ['standards' => ['PSR1']]); + + // Set to values which prevent the test-runner user's `CodeSniffer.conf` file + // from being read and influencing the tests. Also prevent an `exec()` call to stty. + self::setStaticConfigProperty('configData', ['report_width' => 80]); + self::setStaticConfigProperty('configDataFile', ''); + + $config = new Config(); $ruleset = new Ruleset($config); // Default to a file with the same name as the test class. Extension is property based. @@ -74,9 +88,33 @@ public static function tearDownAfterClass() { self::$phpcsFile = null; + // Reset the static properties in the Config class to their defaults to prevent tests influencing each other. + self::setStaticConfigProperty('overriddenDefaults', []); + self::setStaticConfigProperty('executablePaths', []); + self::setStaticConfigProperty('configData', null); + self::setStaticConfigProperty('configDataFile', null); + }//end tearDownAfterClass() + /** + * 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); + $property->setValue(null, $value); + $property->setAccessible(false); + + }//end setStaticConfigProperty() + + /** * Get the token pointer for a target token based on a specific comment found on the line before. *