Tests: stabelize tests using the Config class
#275
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The PHP_CodeSniffer native `Config` class contains a number of static properties. As the value of these static properties will be retained between instantiations of the class, config values set in one test can influence the results for another test, which makes tests unstable. This commit introduces a test "double" of the `Config` class which prevents this from happening. In _most_ cases, tests should be using this class instead of the "normal" Config, with the exception of select tests for the Config class itself.
Note: for the `AbstractSniffUnitTest` class, this change has little to no effect, other than protecting the sniff tests from changes made to the static properties in the `Config` class by the `Core` tests. This is due to the `Config` being cached to a global variable. Fixing that is outside the scope of this PR. Related issues: squizlabs/PHP_CodeSniffer 2899 and 25.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
Nearly every time I work on expanding the Core test suite or adding tests to the Core test suite for a new feature , I run into issues related to the static properties from
Configbleeding through between tests and causing problems.While it can be considered a known issue, it is still annoying and having to work around it each time (and remembering to do so), has lost me enough time by now.
Additionally, there are performance issues with the
Configclass, due to the use of shell commands, likestty/which/whereand file system access. This is not necessarily a problem during a "normal" PHPCS run which only instantiates theConfigclass once, but can have an impact on the test suite where theConfigclass gets instantiated numerous times.These performance issues can be circumvented, but again, doing so is something which needs to be remembered for each test instantiating the
Configclass.The test double I'm now introducing will hopefully prevent/circumvent the majority of these issues for future tests.
Note: there is still an issue with
--sniffsand--excludeCLI arguments not being respected by theRulesetclass when the referenced sniffs are test fixtures, but that's something to be solved another time.Description
Tests: new ConfigDouble test helper class
The PHP_CodeSniffer native
Configclass contains a number of static properties.As the value of these static properties will be retained between instantiations of the class, config values set in one test can influence the results for another test, which makes tests unstable.
This commit introduces a test "double" of the
Configclass which prevents this from happening.In most cases, tests should be using this class instead of the "normal" Config, with the exception of select tests for the Config class itself.
Tests: implement use of the new ConfigDouble class
Note: for the
AbstractSniffUnitTestclass, this change has little to no effect, other than protecting the sniff tests from changes made to the static properties in theConfigclass by theCoretests.This is due to the
Configbeing cached to a global variable. Fixing that is outside the scope of this PR.Related issues: squizlabs/PHP_CodeSniffer#2899 and #25.
Suggested changelog entry
N/A
Related issues/external references
Loosely related to #146
Previous related issues around the performance issues with
Configin the tests: squizlabs/PHP_CodeSniffer#3831 / #61PHPCSUtils PR for the same: PHPCSStandards/PHPCSUtils#550