Don't extend upstream sniffs. #696

Closed
jrfnl opened this Issue Sep 27, 2016 · 6 comments

Projects

None yet

5 participants

@jrfnl
Contributor
jrfnl commented Sep 27, 2016

This is related to a buggy upstream change and has been reported upstream: squizlabs/PHP_CodeSniffer#1177

@jrfnl jrfnl added the upstream label Sep 27, 2016
@jrfnl
Contributor
jrfnl commented Sep 27, 2016 edited

Received response upstream. Quoting it here for further discussion. Highlighting is my doing and gives direction on how to fix this.

This is by design in the new testing system.

If you are testing a sniff like WordPress.Arrays.ArrayDeclaration, and that sniffs produces a WordPress.Arrays.ArrayDeclaration.SingleLineNotAllowed error, you'll need your test to ensure that the error is being produced correctly. The fact that a ruleset excludes it doesn't mean that the sniff doesn't produce it and that it should not be tested. If someone else wants to use that sniff, they'd need to know that the error is being correctly reported.

I suspect what is happening here is that the sniff itself is reporting the error because it is shortcutting the process of array checking by just extending the Squiz sniff. If this is the case, the ruleset should really be referencing the Squiz sniff and excluding the messages it doesn't want. If additional array checks need to be made in the WordPress sniff, the sniff should only report those. Or, the sniff needs needs to only produce the errors that you want.

My main point is that the unit tests are for checking that sniffs work correctly. They aren't for checking that rulesets work correctly.

In other words: WPCS should not extend upstream sniffs, but add them to the ruleset and have a separate sniff for any extra checks which need to be done.

Opinions ?

@jrfnl jrfnl changed the title from FYI: unit tests are failing against PHPCS master to Don't extend upstream sniffs. Sep 27, 2016
@grappler
Contributor

Sounds good. Do we know which sniffs which we need to update?

The one that we need to fix is WordPress.Arrays.ArrayDeclaration to stop the tests from failing.

@westonruter
Member

I'm still pretty fuzzy on what our options are here.

@JDGrimes
Contributor
JDGrimes commented Oct 4, 2016

I think that our options may really depend upon the sniff in question. The issue is that the tests now ignore the ruleset entirely, so any exclusions of rules via the ruleset will not exclude those rules from affecting the tests. At the most basic level, the options are to either update the tests to expect those rules (which don't match our ruleset) or to update the sniffs not to produce those errors to start with. The latter is the direction that it is assumed we will go, since it will mean that the sniffs, the tests, and the ruleset, are harmonious.

So we'll be refactoring the sniffs that:

  • extend a parent sniff
  • and we exclude some of the rules from the parent sniff in our ruleset

This can be done by simply not extending the parent sniff anymore, and adding our custom rules in another sniff. This way we'll keep the exclusion rules in our ruleset, but we'll be referencing the parent sniff to exclude those rules, not our custom sniff. Since the parent sniff will come from upstream, we won't be creating or running tests for it, so we won't get failures from those rules not being excluded when our tests run.

The downside of this approach is that sometimes we are extending a parent sniff that provides a large amount of bootstrap that we can take advantage of, and creating a separate sniff without sub-classing the upstream one will create duplication and also cause a reduction in performance in some cases because more-or-less expensive calculations will have to be performed twice then.

I suppose that at least for some sniffs there would be an alternative, and that would be to modify our child sniff to prevent it from producing those errors at all. This is basically approaching it from the opposite direction, so that we can ultimately remove the exclusion rules from our ruleset, since the sniff will no longer produce them. Probably this method could really be used for all of the sniffs, depending on how hacky we are willing to be.

Probably the best thing to do in some cases will be to just improve the upstream sniff so that it can be more finely configured programmatically.

Note that this doesn't mean that we should no longer extend any upstream sniffs, it only applies when we are running into issues like this, where the parent sniff is providing features that we mute via our ruleset rather than in our sniff child class in some way.

@jrfnl
Contributor
jrfnl commented Oct 4, 2016

I'm looking into fixing this now. It only affects a limited number of sniffs anyhow. (ArrayDeclaration, ValidFunctionName, CastStructure)

@jrfnl
Contributor
jrfnl commented Oct 5, 2016

Ok, PR #703 covers this.

@westonruter westonruter closed this in #703 Oct 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment