WPCS 3.0: Fix ruleset tests #736
Conversation
Add support for PHP 8.1 features, including fixing a retokenization of reserved keywords bug in PHPCS 3.7.0.
Brings them more into line with the Composer scripts in WPCS.
VIPCS needs to be compatible with WordPress Coding Standards 3.0.0, which is currently in development on its `develop` branch. WPCS now uses some extra dependencies, which are also in development, so we need to allow for a minimum-stability of dev. Run `composer update -W` to pull those extra dependencies in.
Uses the composer-normalize plugin to keep a consistent order.
WordPress-Extra will no longer use the `Generic.Arrays.DisallowShortArraySyntax` rule, but does use the improved `Universal.Arrays.DisallowShortArraySyntax` from PHPCSExtra instead, so whilst we have short array syntax being used in the VIPCS sniffs, let's continue to exclude the rule.
The previous `Sniff::merge_custom_array()` function was moved to a new Helper class in WPCS: WordPress/WordPress-Coding-Standards#2157. However, it is marked as non-public (only meant for WPCS directly), so copying into the VIPCS sniff is easiest for now.
This is more tested than the WPCS Sniff::addMessage().
VIPCS makes use of PHPCSUtils directly (such as for MessageHelper), so it should be marked as such in the Composer config.
Shows up empty in Ci and locally.
The ruleset tests were failing mainly due to WordPress/WordPress-Coding-Standards#2108 which renamed some violation codes that we were excluding. The other change to account for was the replacement of a WPCS StrictComparisons sniff with a PHPCSExtras Universal sniff. The sniff in PHPCSExtra contains a fixer. As this is a risky fixer, this fixer is turned off for WPCS. The sniff in PHPCSExtra will provide metrics about loose versus strict comparisons.
Make it clear which ruleset being tested, even on a test failure.
Validate the PHPCS installed standards in the quick test.
GaryJones
force-pushed
the
fix/ruleset-tests
branch
from
b72bb6b
to
ce7d0c4
Compare
There was a problem hiding this comment.
Reviewed with the exception that I haven't done a detailed review of the last commit (see below).
Along the same lines as previous reviews:
- Commit 1 ("Ruleset tests: update for WPCS 3.0") should be in the same PR as the commit changing the minimum WPCS version to 3.x in Composer as as soon as 3.x becomes the minimum, the rulesets becomes invalid without the changes from this commit.
- Commit 2 + 4 ("Ruleset tests: Add label before test runs" and "Ruleset tests: Update label") should be squashed together.
- Commit 3 shouldn't exist (nor should the "Disable ruleset test" commit in PR WPCS 3.0: Fix incompatibilities by using PHPCSUtils #734)
- Commit 5 ("Add PHPCSExtras dependency") doesn't belong with this PR and it is unclear why you are adding PHCPSExtra.
For those sniffs in use by WPCS, the dependency is inherited from WPCS.
If VIPCS is not directly using PHPCSExtra (adding sniffs from PHPCSExtra which are not used by WPCS), the dependency is not "our" dependency. - Commit 6 ("CI: Use WPCS develop branch for quick test") should be combined with the "CI: Refresh unit tests workflow" commit from PR WPCS 3.0: Fix incompatibilities by using PHPCSUtils #734 and added to PR WPCS 3.0: Composer updates #732 as those changes are directly related to the change in the WPCS dependency version.
| @@ -546,7 +546,7 @@ str_replace( 'foo', array( 'bar', 'foo' ), 'foobar' ); // Error. | |||
|
|
|||
| // WordPressVIPMinimum.Security.Underscorejs | |||
| echo "<script> | |||
| _.templateSettings = { | |||
| _.templateSettings = { | |||
There was a problem hiding this comment.
Was this an intentional or accidental trailing whitespace ? I.e. are you changing what's being tested now or not ?
(same question for the same change in WordPressVIPMinimum/ruleset-test.inc)
| @@ -98,6 +98,9 @@ public function __construct( $ruleset, $expected = [] ) { | |||
| $this->phpcs_bin = realpath( $phpcs_bin ); | |||
| } | |||
|
|
|||
| // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped | |||
There was a problem hiding this comment.
There are quite a few ignore annotations of this sniff and the system_calls group of the WordPress.PHP.DiscouragedPHPFunctions sniff for this file.
Would it make sense to add an exception to the project phpcs.xml.dist file for those two rules for this particular file (and to remove the inline ignore annotations) ?
Alternatively, // phpcs:disable comments could be added to the top of this particular file for those error codes.
| @@ -91,6 +86,9 @@ jobs: | |||
| composer-options: --ignore-platform-reqs | |||
| custom-cache-suffix: $(date -u -d "-0 month -$(($(date +%d)-1)) days" "+%F") | |||
|
|
|||
| - name: Display PHPCS installed standards | |||
There was a problem hiding this comment.
This doesn't validate anything. It just shows the standards, it doesn't error out if expected standards would not be registered.
Also, this information is already available at the bottom of the "Install Composer dependencies" step as the Composer PHPCS plugin will display it as part of its user facing output.
|
Closing for now, as #746 handled the fixing of ruleset tests (and other bits) for PHP 8.1 while continuing to use WPCS 2.3.0. Some of the changes (with feedback) in this PR will be pulled into a branch that uses WPCS 3.0. |
Once #734 has been merged, this can be rebased to just show the ruleset tests changes.
Ruleset tests: update for WPCS 3.0
The ruleset tests were failing mainly due to the renaming of some violation codes that we were excluding.
The other change to account for was the replacement of a WPCS StrictComparisons sniff with a PHPCSExtras Universal sniff. The sniff in PHPCSExtra contains a fixer. As this is a risky fixer, this fixer is turned off for WPCS. The sniff in PHPCSExtra will provide metrics about loose versus strict comparisons.
Ruleset tests: Add label before test runs
Make it clear which ruleset being tested, even on a test failure.
Enable ruleset tests
Now the issues have been address, these can be enabled again in
composer check-alland a Github workflow.