Skip to content

Ruleset: remove test specific code #996

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 2 commits into from
Apr 15, 2025

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Apr 15, 2025

Description

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

Tests: remove work-arounds for --sniffs not being respected

Now that the --sniffs CLI argument will be respected in the tests, a number of work-arounds previously introduced can be removed.

Note: there may be a few more work-arounds which can be removed, like removing some fixture rulesets in favour of test inline sniff selection, but this should be a good first step.

Suggested changelog entry

Changed:
The Ruleset class no longer has special behaviour when used in a test context.

jrfnl added 2 commits April 15, 2025 14:34
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
Now that the `--sniffs` CLI argument will be respected in the tests, a number of work-arounds previously introduced can be removed.

Note: there may be a few more work-arounds which can be removed, like removing some fixture rulesets in favour of test inline sniff selection, but this should be a good first step.
@jrfnl jrfnl merged commit f607f3c into 4.x Apr 15, 2025
54 checks passed
@jrfnl jrfnl deleted the phpcs-4.0/feature/test-code-should-be-in-tests branch April 15, 2025 12:55
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.

2 participants