Skip to content

Commit 41d955a

Browse files
committed
Ruleset: remove test specific code
The PHPCS 2.7.1 release introduced a change to how sniff tests are run. Originally, sniff tests would be run in the context of their ruleset. This could lead to parts of the sniff being untestable, like if the ruleset would exclude a error code or contained custom property settings. As of PHPCS 2.7.1, sniff tests are run in isolation and no longer take the ruleset of the standard they belong to into account, which solves the above problem. This change was introduced via commit 3df85dc. Unfortunately, the way this change was made was via a change to the `Ruleset` class, not by changing the test framework. It basically meant that the `Ruleset` class would now behave differently when the following two conditions were met: 1. It was being used in a test context (`PHP_CODESNIFFER_IN_TESTS` constant defined). 2. Sniff restrictions were applied via the Config, either by using the CLI `--sniffs` argument or by setting the `Config::$sniffs` property directly. In other words, this now created a new problem when testing parts of the `Ruleset` class which would need the `Config::$sniffs` property to be set. It also makes writing various other tests for the framework more difficult as there are plenty of times when tests would benefit from the `Config::$sniffs` property being respected. In other words, this led to work-arounds being needed in various other places in the tests, either by emulating the sniff selection or by bypassing the `Ruleset` conditions and doing the sniff selection in a test specific ruleset XML fixture file. Aside from the above, it also makes the barrier to entry for contributors to write tests for PHPCS that much higher as the behaviour is non-intuitive. All in all, time to get rid of the condition in the `Ruleset` class. Now, of course, we do still want to test sniffs in isolation, so to do that the `AbstractSniffUnitTest` class has been updated to on-the-fly create ruleset XML files which only include a single sniff without any customizations. I considered using `php://memory` or `php://temp` instead of writing the file to disk, however, neither of those work with `file_get_contents()`, which is used to read the XML ruleset files in the `Ruleset` class, so unfortunately, those temporary file writes cannot be avoided without making bigger changes, which is outside the scope of this PR. Now, I wondered about the performance impact of writing a file to disk for every sniff being tested, and while there is a small, but noticeable, difference when running on Windows (20 vs 27 seconds for `--filter Standards`), IMO this difference is acceptable when offset against the hours and hours of dev time lost trying to debug why perfectly valid tests weren't working due to `--sniffs` not being respected in the tests. And to be sure, I've also tested the new test isolation mechanism with an external standard which is using the PHPCS native test framework and it looks to be working fine. Note: even though the `AbstractSniffUnitTest` will clean up the temporary file after each test, if the test run would crash, it could be possible for the file to remain on disk. With this in mind, the file name for the temporary file has been added to the `.gitignore` file. Related to 966
1 parent 7bc2ff3 commit 41d955a

File tree

3 files changed

+65
-45
lines changed

3 files changed

+65
-45
lines changed

.gitignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,3 +8,4 @@
88
composer.lock
99
phpstan.neon
1010
/node_modules/
11+
/tests/Standards/sniffStnd.xml

src/Ruleset.php

Lines changed: 0 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -201,22 +201,6 @@ public function __construct(Config $config)
201201
Autoload::addSearchPath(dirname($standard), $namespace);
202202
}
203203

204-
if (defined('PHP_CODESNIFFER_IN_TESTS') === true && empty($restrictions) === false) {
205-
// In unit tests, only register the sniffs that the test wants and not the entire standard.
206-
foreach ($restrictions as $restriction) {
207-
$sniffs = array_merge($sniffs, $this->expandRulesetReference($restriction, dirname($standard)));
208-
}
209-
210-
if (empty($sniffs) === true) {
211-
// Sniff reference could not be expanded, which probably means this
212-
// is an installed standard. Let the unit test system take care of
213-
// setting the correct sniff for testing.
214-
return;
215-
}
216-
217-
break;
218-
}
219-
220204
if (PHP_CODESNIFFER_VERBOSITY === 1) {
221205
echo "Registering sniffs in the $standardName standard... ";
222206
if (count($config->standards) > 1 || PHP_CODESNIFFER_VERBOSITY > 2) {

tests/Standards/AbstractSniffUnitTest.php

Lines changed: 64 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -26,18 +26,43 @@ abstract class AbstractSniffUnitTest extends TestCase
2626
{
2727

2828
/**
29-
* Cache for the Config object.
29+
* Ruleset template with placeholders.
3030
*
31-
* @var \PHP_CodeSniffer\Tests\ConfigDouble
31+
* @var string
3232
*/
33-
private static $config;
33+
private const RULESET_TEMPLATE = <<<'TEMPLATE'
34+
<?xml version="1.0"?>
35+
<ruleset xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" name="[STANDARDNAME]" xsi:noNamespaceSchemaLocation="../../phpcs.xsd">
36+
<description>Temporary ruleset used by the AbstractSniffUnitTest class.</description>
37+
38+
<rule ref="[SNIFFFILEREF]"/>
39+
40+
</ruleset>
41+
TEMPLATE;
42+
43+
/**
44+
* Placeholders used in the ruleset template which need to be replaced.
45+
*
46+
* @var array<string>
47+
*/
48+
private const SEARCH_FOR = [
49+
'[STANDARDNAME]',
50+
'[SNIFFFILEREF]',
51+
];
52+
53+
/**
54+
* Location where the temporary ruleset file will be saved.
55+
*
56+
* @var string
57+
*/
58+
private const RULESET_FILENAME = __DIR__.'/sniffStnd.xml';
3459

3560
/**
36-
* Cache for Ruleset objects.
61+
* Cache for the Config object.
3762
*
38-
* @var array<string, \PHP_CodeSniffer\Ruleset>
63+
* @var \PHP_CodeSniffer\Tests\ConfigDouble
3964
*/
40-
private static $rulesets = [];
65+
private static $config;
4166

4267
/**
4368
* Extensions to disregard when gathering the test files.
@@ -52,6 +77,18 @@ abstract class AbstractSniffUnitTest extends TestCase
5277
];
5378

5479

80+
/**
81+
* Clean up temporary ruleset file.
82+
*
83+
* @return void
84+
*/
85+
public static function tearDownAfterClass(): void
86+
{
87+
@unlink(self::RULESET_FILENAME);
88+
89+
}//end tearDownAfterClass()
90+
91+
5592
/**
5693
* Get a list of all test files to check.
5794
*
@@ -103,7 +140,8 @@ protected function shouldSkipTest()
103140
* Tests the extending classes Sniff class.
104141
*
105142
* @return void
106-
* @throws \PHPUnit\Framework\Exception
143+
*
144+
* @throws \PHP_CodeSniffer\Exceptions\RuntimeException
107145
*/
108146
final public function testSniff()
109147
{
@@ -122,6 +160,23 @@ final public function testSniff()
122160
// Get a list of all test files to check.
123161
$testFiles = $this->getTestFiles($testFileBase);
124162

163+
$sniffFile = preg_replace('`[/\\\\]Tests[/\\\\]`', DIRECTORY_SEPARATOR.'Sniffs'.DIRECTORY_SEPARATOR, $testFileBase);
164+
$sniffFile = str_replace('UnitTest.', 'Sniff.php', $sniffFile);
165+
166+
if (file_exists($sniffFile) === false) {
167+
$this->fail(sprintf('ERROR: Sniff file %s for test %s does not appear to exist', $sniffFile, static::class));
168+
}
169+
170+
$replacements = [
171+
$standardName,
172+
$sniffFile,
173+
];
174+
$rulesetContents = str_replace(self::SEARCH_FOR, $replacements, self::RULESET_TEMPLATE);
175+
176+
if (file_put_contents(self::RULESET_FILENAME, $rulesetContents) === false) {
177+
throw new RuntimeException('Failed to write custom ruleset file');
178+
}
179+
125180
if (isset(self::$config) === true) {
126181
$config = self::$config;
127182
} else {
@@ -130,31 +185,11 @@ final public function testSniff()
130185
self::$config = $config;
131186
}
132187

133-
$config->standards = [$standardName];
188+
$config->standards = [self::RULESET_FILENAME];
134189
$config->sniffs = [$sniffCode];
135190
$config->ignored = [];
136191

137-
if (isset(self::$rulesets[$standardName]) === false) {
138-
$ruleset = new Ruleset($config);
139-
self::$rulesets[$standardName] = $ruleset;
140-
}
141-
142-
$ruleset = self::$rulesets[$standardName];
143-
144-
$sniffFile = preg_replace('`[/\\\\]Tests[/\\\\]`', DIRECTORY_SEPARATOR.'Sniffs'.DIRECTORY_SEPARATOR, $testFileBase);
145-
$sniffFile = str_replace('UnitTest.', 'Sniff.php', $sniffFile);
146-
147-
if (file_exists($sniffFile) === false) {
148-
$this->fail(sprintf('ERROR: Sniff file %s for test %s does not appear to exist', $sniffFile, static::class));
149-
}
150-
151-
$sniffClassName = substr(get_class($this), 0, -8).'Sniff';
152-
$sniffClassName = str_replace('\Tests\\', '\Sniffs\\', $sniffClassName);
153-
$sniffClassName = Common::cleanSniffClass($sniffClassName);
154-
155-
$restrictions = [strtolower($sniffClassName) => true];
156-
$ruleset->registerSniffs([$sniffFile], $restrictions, []);
157-
$ruleset->populateTokenListeners();
192+
$ruleset = new Ruleset($config);
158193

159194
$failureMessages = [];
160195
foreach ($testFiles as $testFile) {

0 commit comments

Comments
 (0)