Skip to content
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

Travis: add a new check for early detection of ruleset problems #1430

Merged
merged 1 commit into from
Jul 24, 2018

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Jul 24, 2018

WPCS itself uses WordPress-Extra + WordPress-Docs for its CS, which includes WordPress-Core.

Those rulesets were therefore "tested" on each build, just by running them against the WPCS code.
However, the WPCS native CS ruleset also excludes some things and the CS check ignores warnings for the Travis run and it also left the WordPress-VIP and WordPress rulesets untested.

This PR intends to fix this by adding a minimal test file which should trigger most sniffs and testing each ruleset against it.

This should, from now on, prevent the ruleset issues we've seen related to the moving and deprecating of sniffs, i.e. #1325 and #1425.
This will also serve as an early warning system in case any of the upstream sniffs which are included in the rulesets would be removed/renamed or otherwise have a change in behaviour which WPCS will need to take into account.

Notes:

  • This is complimentary to the unit tests as those are run in a ruleset-agnostic manner, i.e. the ruleset isn't loaded and any settings in the ruleset are ignored.
  • At this moment, the test file is incomplete, but complete enough for our purposes. Further improvement of the test file in the future to make sure that all sniffs are triggered is recommended.
  • The check only needs to be run once against every PHPCS version supported/tested.
    With that in mind, "old-style" ignore annotations have been used.
    Once the minimum PHPCS version WPCS supports goes beyond PHPCS 3.2.0, those should be swopped out for selective ignores instead.
  • The check should be run on a high PHP version to prevent triggering the Generic.PHP.Syntax sniff when modern PHP code is included in the test file to ensure that this triggers the appropriate sniffs.
    At this moment, the most modern syntax included in nullable types which was introduced in PHP 7.1 which is why that is the PHP version against which these tests are currently run.

N.B.: The builds for this PR won't pass until the current issue with the VIP deprecation warnings (#1425) is fixed, which is, of course, exactly the point of this PR ;-)

@@ -10,6 +10,7 @@
<!-- Exclude the code in the PHPCS 2.x test files copied in from PHPCS. -->
<exclude-pattern>/Test/AllTests.php</exclude-pattern>
<exclude-pattern>/Test/Standards/*.php</exclude-pattern>
<exclude-pattern>/bin/class-ruleset-test.php</exclude-pattern>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bin/ is typically for shell scripts or other binaries.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be happy to place it somewhere else, any suggestions on a more appropriate location ?

The /Test directory came to mind, though all the other files in that directory will be removed in WPCS 2.x when PHPCS 2.x is dropped and the test files would need another name if placed there so as not to confuse PHPUnit.

WPCS itself uses `WordPress-Extra` + `WordPress-Docs` for its CS, which includes `WordPress-Core`.

Those rulesets were therefore "tested" on each build, just by running them against the WPCS code.
However, the WPCS native CS ruleset also excludes some things and the CS check ignores warnings for the Travis run and it also left the `WordPress-VIP` and `WordPress` rulesets _untested_.

This PR intends to fix this by adding a minimal test file which should trigger most sniffs and testing each ruleset against it.

This should, from now on, prevent the ruleset issues we've seen related to the moving and deprecating of sniffs.
This will also serve as an early warning system in case any of the upstream sniffs which are included in the rulesets would be removed/renamed or otherwise have a change in behaviour which WPCS will need to take into account.

Notes:
* This is complimentary to the unit tests as those are run in a ruleset-agnostic manner, i.e. the ruleset isn't loaded and any settings in the ruleset are ignored.
* At this moment, the test file is incomplete, but complete enough for our purposes.
    Further improvement of the test file in the future to make sure that all sniffs are triggered is recommended.
* The check only needs to be run once against every PHPCS version supported/tested.
    With that in mind, "old-style" ignore annotations have been used.
    Once the minimum PHPCS version WPCS supports goes beyond PHPCS 3.2.0, those should be swopped out for selective ignores instead.
* The check should be run on a high PHP version to prevent triggering the `Generic.PHP.Syntax` sniff when modern PHP code is included in the test file to ensure that this triggers the appropriate sniffs.
    At this moment, the most modern syntax included in nullable types which was introduced in PHP 7.1 which is why that is the PHP version against which these tests are currently run.
@jrfnl jrfnl force-pushed the feature/travis-add-ruleset-check branch from 90fcca8 to 3d92e2f Compare July 24, 2018 21:36
@jrfnl
Copy link
Member Author

jrfnl commented Jul 24, 2018

Rebased so we can get a passing build. I'll merge once the build is green.

@jrfnl jrfnl merged commit 1a9d8f8 into develop Jul 24, 2018
@jrfnl jrfnl deleted the feature/travis-add-ruleset-check branch July 24, 2018 21:53
@jrfnl jrfnl mentioned this pull request Jul 24, 2018
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.

3 participants