Skip to content

Config: remove more test specific conditions #1068

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.

Sign up for GitHub

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
jrfnl merged 1 commit into from
May 2, 2025
Merged

Config: remove more test specific conditions #1068

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Config: remove more test specific conditions 
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
  • Loading branch information
@jrfnl
jrfnl committed May 1, 2025
commit 3d54d4c6006f9834337059aab9fcf653b65470cf
26 changes: 10 additions & 16 deletions src/Config.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -650,16 +650,14 @@ public function restoreDefaults()
$this->colors = (bool) $colors;
}

if (defined('PHP_CODESNIFFER_IN_TESTS') === false) {
$cache = self::getConfigData('cache');
if ($cache !== null) {
$this->cache = (bool) $cache;
}
$cache = self::getConfigData('cache');
if ($cache !== null) {
$this->cache = (bool) $cache;
}

$parallel = self::getConfigData('parallel');
if ($parallel !== null) {
$this->parallel = max((int) $parallel, 1);
}
$parallel = self::getConfigData('parallel');
if ($parallel !== null) {
$this->parallel = max((int) $parallel, 1);
}

}//end restoreDefaults()
Expand Down Expand Up @@ -817,10 +815,8 @@ public function processLongArgument($arg, $pos)
break;
}

if (defined('PHP_CODESNIFFER_IN_TESTS') === false) {
$this->cache = true;
$this->overriddenDefaults['cache'] = true;
}
$this->cache = true;
$this->overriddenDefaults['cache'] = true;
break;
case 'no-cache':
if (isset($this->overriddenDefaults['cache']) === true) {
Expand Down Expand Up @@ -928,9 +924,7 @@ public function processLongArgument($arg, $pos)

$this->exclude = $this->parseSniffCodes(substr($arg, 8), 'exclude');
$this->overriddenDefaults['exclude'] = true;
} else if (defined('PHP_CODESNIFFER_IN_TESTS') === false
&& substr($arg, 0, 6) === 'cache='
) {
} else if (substr($arg, 0, 6) === 'cache=') {
if ((isset($this->overriddenDefaults['cache']) === true
&& $this->cache === false)
|| isset($this->overriddenDefaults['cacheFile']) === true
Expand Down