From b1205ec96da677370230cd66982784c319795977 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 19 Apr 2025 02:32:51 +0200 Subject: [PATCH] Generic/ScopeIndent: remove check for test constant from src code The `Generic.WhiteSpace.ScopeIndent` sniff includes a `private` boolean `$debug` property which can be toggled to enable/disable showing sniff specific debug information. By default, debug information is not shown. Showing of debug information can be enabled in the following manner: 1. By explicitly changing the default value of the property to `true` in the sniff file (without committing as the default should not change in the committed code). 2. By running with the `--runtime-set scope_indent_debug 1` flag set. 3. By setting the `scope_indent_debug` config setting to `true` from a custom ruleset. 4. By setting a system-wide default for the `scope_indent_debug` config setting by running `--config-set scope_indent_debug 1`. The `Generic.WhiteSpace.ScopeIndent` sniff includes a condition in the `register()` method to prevent debug information from being generated during a test run, which would lead to "unexpected output", which would mark the test as risky and potentially fail the build. Now, generally speaking, having conditions specific to running the tests in the source should be consider bad practice as it makes testing code affected by those type of conditions impossible. Basically, if a test needs a specific environment, this should be handled in the test setup/teardown. As things were, this condition was also not very effective. Of the above ways of changing the value of the `$debug` property, only method [1] and [4] could come into play in automated test run situations and the debug output would only be prevented for [1] (the hard-coded default value being changed in the sniff code itself). In case of [4], the debug info would (originally) still show. Now, the debug info showing when the system-wide default was set for the `scope_indent_debug` config setting [4], was (incidentally) fixed via PR 275 which introduced the `ConfigDouble`, which prevents the user specific `CodeSniffer.conf` file from being read out for those tests using the `ConfigDouble`, which, since 275, includes all tests extending the `AbstractSniffUnitTest` class. So, now we still have situation [1] to deal with - which basically comes down to a maintainer/contributor enabling the debug option while working on the sniff, but not wanting the tests to fail when running them to check the changing they are making. Well, for the sniff tests, as they currently, we cannot use the PHPUnit `setUp()` method as the sniff will not have been initialized yet, preventing us from using reflection on the object. Additionally, depending on the order of tests being run, the `Config` may also not yet have been setup. That is inherit to the current test setup of the `AbstractSniffUnitTest` class and fixing that is outside the scope of this ticket. However, the `AbstractSniffUnitTest` offers a `setCliValues()` method, which is called before the test case files are being tokenized and processed, so we can use that method to set the `scope_indent_debug` config to `false` for the test run, solving the problem and allowing for removing the test specific condition from the `ScopeIndent` sniff. Additional notes: * The overruling of the config setting is done in the test files for both the `Generic` as well as the `PEAR` `ScopeIndent` sniffs as the `PEAR` sniff extends the `Generic` sniff and we cannot rely on the tests being run in a specific order, so we need to make sure the config is set either way. * As things are, the sniff calls `Config::getConfigData('scope_indent_debug')` at the start of the `process()` method. This should be moved up into the `register()` method for efficiency, as Config settings are not changable during the run, so there should be no need to check the value for each and every file. However, this would break with how the current `AbstractSniffUnitTest` base class works as it does everything within the test method. Fixing that is outside the scope of this PR, but something to keep in mind for a later time. Related to 966 --- .../Sniffs/WhiteSpace/ScopeIndentSniff.php | 4 ---- .../Tests/WhiteSpace/ScopeIndentUnitTest.php | 2 ++ .../PEAR/Tests/WhiteSpace/ScopeIndentUnitTest.php | 15 +++++++++++++++ 3 files changed, 17 insertions(+), 4 deletions(-) diff --git a/src/Standards/Generic/Sniffs/WhiteSpace/ScopeIndentSniff.php b/src/Standards/Generic/Sniffs/WhiteSpace/ScopeIndentSniff.php index 5c7df9a470..d587d79669 100644 --- a/src/Standards/Generic/Sniffs/WhiteSpace/ScopeIndentSniff.php +++ b/src/Standards/Generic/Sniffs/WhiteSpace/ScopeIndentSniff.php @@ -106,10 +106,6 @@ class ScopeIndentSniff implements Sniff */ public function register() { - if (defined('PHP_CODESNIFFER_IN_TESTS') === true) { - $this->debug = false; - } - return [T_OPEN_TAG]; }//end register() diff --git a/src/Standards/Generic/Tests/WhiteSpace/ScopeIndentUnitTest.php b/src/Standards/Generic/Tests/WhiteSpace/ScopeIndentUnitTest.php index fc9f9a8b92..02bb4cd996 100644 --- a/src/Standards/Generic/Tests/WhiteSpace/ScopeIndentUnitTest.php +++ b/src/Standards/Generic/Tests/WhiteSpace/ScopeIndentUnitTest.php @@ -30,6 +30,8 @@ final class ScopeIndentUnitTest extends AbstractSniffUnitTest */ public function setCliValues($testFile, $config) { + $config->setConfigData('scope_indent_debug', false, true); + // Tab width setting is only needed for the tabbed file. if ($testFile === 'ScopeIndentUnitTest.2.inc') { $config->tabWidth = 4; diff --git a/src/Standards/PEAR/Tests/WhiteSpace/ScopeIndentUnitTest.php b/src/Standards/PEAR/Tests/WhiteSpace/ScopeIndentUnitTest.php index 3c6fd4a5e5..b6a791e28c 100644 --- a/src/Standards/PEAR/Tests/WhiteSpace/ScopeIndentUnitTest.php +++ b/src/Standards/PEAR/Tests/WhiteSpace/ScopeIndentUnitTest.php @@ -20,6 +20,21 @@ final class ScopeIndentUnitTest extends AbstractSniffUnitTest { + /** + * Get a list of CLI values to set before the file is tested. + * + * @param string $testFile The name of the file being tested. + * @param \PHP_CodeSniffer\Config $config The config data for the test run. + * + * @return void + */ + public function setCliValues($testFile, $config) + { + $config->setConfigData('scope_indent_debug', false, true); + + }//end setCliValues() + + /** * Returns the lines where errors should occur. *