Skip to content

Generic/ScopeIndent: remove check for test constant from src code #1036

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 19, 2025

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Apr 19, 2025

Description

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.

Suggested changelog entry

N/A

Related issues/external references

Related to #966

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
@jrfnl jrfnl added this to the 3.13.0 milestone Apr 19, 2025
@jrfnl jrfnl merged commit 1229916 into master Apr 19, 2025
63 checks passed
@jrfnl jrfnl deleted the feature/966-generic-scopeindent-remove-phpcs-in-tests branch April 19, 2025 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant