Config: remove more test specific conditions#1068
Merged
Conversation
The `--cache` option could not be tested as the `Config` class did not allow for it to be set in a test situation. This "conditional ignore" was introduced in commit 24c7a7f around the same time the caching feature was introduced. The commit message doesn't give a reason for introducing the conditional ignore. I can only guess why and my guess would be that it was to prevent test runs being influence by dev-user specific system-wide defaults from a `CodeSniffer.conf` file. In my opinion, that is not a good reason for keeping the "conditional ignore". * First of all, the default setting is for the cache to be _off_, so by default, tests wouldn't use the cache anyhow. * Second of all, the problem of interference from dev-user specific system-wide defaults was fixed by the introduction of the `ConfigDouble` and the `AbstractRealConfigTestCase`. All in all, I think these conditions can be safely removed. Do keep in mind though that subsequent test runs for tests involving caching may re-use the cache from an earlier run test. To prevent that, set an explicit cacheFile for the test using `--cache=<filename>` and remove this cache file after running the test(s) via the `tearDown[AfterClass]()` method. Note: one of the removed conditions also affected the `--parallel` option, but only for system-wide settings, not for CLI arguments. Related to 966
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Description
The
--cacheoption could not be tested as theConfigclass did not allow for it to be set in a test situation.This "conditional ignore" was introduced in commit 24c7a7f around the same time the caching feature was introduced. The commit message doesn't give a reason for introducing the conditional ignore. I can only guess why and my guess would be that it was to prevent test runs being influence by dev-user specific system-wide defaults from a
CodeSniffer.conffile.In my opinion, that is not a good reason for keeping the "conditional ignore".
ConfigDoubleand theAbstractRealConfigTestCase.All in all, I think these conditions can be safely removed.
Do keep in mind though that subsequent test runs for tests involving caching may re-use the cache from an earlier run test.
To prevent that, set an explicit cacheFile for the test using
--cache=<filename>and remove this cache file after running the test(s) via thetearDown[AfterClass]()method.Note: one of the removed conditions also affected the
--paralleloption, but only for system-wide settings, not for CLI arguments.Suggested changelog entry
N/A
Related issues/external references
Related to #966