Bug: change in test runner breaks tests with exclusions

[jrfnl]

The change in commit 3df85dc breaks tests for rulesets which include a rule, but exclude a modular sub-sniff.

The exclusion is no longer respected and results in unit test run failures such as: https://travis-ci.org/WPTRT/WordPress-Coding-Standards/jobs/163087045#L188.

Minimal code example to reproduce the issue:

Ruleset ruleset.xml:

    <rule ref="WordPress.Arrays.ArrayDeclaration">
        <exclude name="WordPress.Arrays.ArrayDeclaration.SingleLineNotAllowed" />
    </rule>

Sniff file WordPress/Sniffs/Arrays/ArrayDeclarationSniff.php

class WordPress_Sniffs_Arrays_ArrayDeclarationSniff extends Squiz_Sniffs_Arrays_ArrayDeclarationSniff {
    // Some additional checks on top of the Squiz sniff.
}

Test case WordPress/Tests/Arrays/ArrayDeclarationUnitTest.inc:

// Multiple values in an array on a single line is allowed.
array( 1, 2 );

Test case fixed file WordPress/Tests/Arrays/ArrayDeclarationUnitTest.inc.fixed:

// Multiple values in an array on a single line is allowed.
array( 1, 2 );

Failure details:

There was 1 failure:
1) WordPress_Tests_Arrays_ArrayDeclarationUnitTest::testSniff
[LINE 205] Expected 0 error(s) in ArrayDeclarationUnitTest.inc but found 1 error(s). The error(s) found were:
-> Array with multiple values cannot be declared on a single line (WordPress.Arrays.ArrayDeclaration.SingleLineNotAllowed)
[LINE 208] Expected 2 error(s) in ArrayDeclarationUnitTest.inc but found 3 error(s). The error(s) found were:
-> Array with multiple values cannot be declared on a single line (WordPress.Arrays.ArrayDeclaration.SingleLineNotAllowed)
-> Expected 1 space after array opener, found 3. (WordPress.Arrays.ArrayDeclaration.SpaceAfterArrayOpener)
-> Expected 1 space before array closer, found 5. (WordPress.Arrays.ArrayDeclaration.SpaceAfterArrayCloser)

Fixed version of ArrayDeclarationUnitTest.inc does not match expected version in ArrayDeclarationUnitTest.inc.fixed; the diff is
--- WordPress/Tests/Arrays/ArrayDeclarationUnitTest.inc.fixed
+++ PHP_CodeSniffer
@@ -202,7 +202,13 @@
);
// Multiple values in an array on a single line is allowed.
-array( 1, 2 );
+array(
+ 1,
+2,
+);

// Test for fixing of extra whitespace.
-array( 1, 2 );
+array(
+ 1,
+2,
+);

[gsherwood]

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.

[gsherwood]

Sorry, I forgot to make my final point before submitting that comment. My main point is that the unit tests are for checking that sniffs work correctly. They aren't for checking that rulesets work correctly.

[jrfnl]

Thanks for your repsonse. Makes sense.

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.

I think this is the direction these kind of sniffs should be taking. Will look into refactoring those.

[gsherwood]

I think this is the direction these kind of sniffs should be taking.

The Squiz array sniff is a horrible sniff because it is all just a big lump. I have plans, that I can never seem to get around to, for creating a bunch of new Generic sniffs to check those common individual rules. Then probably a new one to enforce a simpler array indent rule rather than the convoluted one the Squiz standard uses now. It's one of the original sniffs from before PHPCS was a public project, so it needs some love.

[jrfnl]

I hear you, if only days had 40 hours each ;-) Hmm.. we'd probably still have the same struggle for time....

FYI: I'm working on a number of extra utility functions, things like isFunctionCall(), getFunctionCallParameter(), doesTokenHaveScope(), getNamespace(). If you're interested, I'd like to upstream them to PHPCS once they have been tested in other standards.
These kind of functions can get rid of quite some duplicate sniff code and simplify maintenance. Might help.
Let me know if you like the idea.

[gsherwood]

Let me know if you like the idea.

I sure do.