WordPressVIPMinimum: fix the failing ruleset test & let Travis show failures

[jrfnl]

Ruleset test script: fail the build on failing ruleset tests

The builds on PHP 5.4, 5.5 and 5.6 should have been failing on the ruleset test for a while now, but aren't.

Example: https://travis-ci.org/github/Automattic/VIP-Coding-Standards/jobs/710513865#L311-L315

What is happening here is that when several commands are run via a script, Travis will only look at the exit code of the last script to decide whether the build should be failed or not.

This means that failures from the WordPressVIPMinimum ruleset test were never failing the build as they were hidden by a passing ruleset test for the WordPress-VIP-Go ruleset.

By chaining the commands together, the exit codes of both commands are taken into account when determining whether the build should be failed or not.

WordPressVIPMinimum: fix the failing ruleset test

Ok, this is a fun one.

Line 5 of the ruleset-test is not supposed to error. It is just setting up the variables for the VariableAnalysis test.

Line 7, however, is supposed to error on a PHP parse error (Generic.PHP.Syntax).

The above is true and working for PHP 7.

However on PHP 5, line 7 will not throw a parse error, while line 5 actually will - Cannot re-assign $this.

As the test on line 7 is about checking that the parse error sniff is correctly triggered and the error on line 5 on PHP 5 is a parse error, I deem this sufficiently covered and have fixed the build failure by making the expected errors on line 5 and 7 conditional on the PHP version on which the tests are being run.

Includes annotating these findings in the inline documentation of the test case file.

Fun times ;-)

RulesetTest: fix the test script to be able to handle 0 values

Turns out the test script was not set up to be able to deal with 0 values and would throw a very informative "Expected 0 errors, found 0 on line 7." error in such a case.

Fixed now.

[GaryJones]

In the WordPress-VIP-Go ruleset test, it looks like I changed $this = new stdClass() to $obj = '', and then updated the references later in the file from $this to $obj. I would be fine with that happening for the WordPressVIPMinimum ruleset test as well, as it looks like this got overlooked (since Travis wasn't reporting it as a failure), rather than introducing any PHP version conditions than are necessary.

[jrfnl]

In the WordPress-VIP-Go ruleset test, it looks like I changed $this = new stdClass() to $obj = '', and then updated the references later in the file from $this to $obj.

Except that in the line in the WordPressVIPMinimum test case file, both $obj as well as $this are defined, so renaming $this to $obj would effectively overwrite $obj later on the same line, so I suspect we'd be undoing part of the test that way.

I can, of course, rename it to something else, $stdClass comes to mind. Agreed ?

I do think the other changes related to the potential for 0 values in the lists should remain to make the script more resilient for future changes.

[jrfnl]

@GaryJones Oh and another thing - that still wouldn't take care of the fact that PHP 5 does not throw a parse error on line 7.

[GaryJones]

Except that in the line in the WordPressVIPMinimum test case file, both $obj as well as $this are defined, so renaming $this to $obj would effectively overwrite $obj later on the same line, so I suspect we'd be undoing part of the test that way.

In which case, I think the definition of $this could be removed, and any later usages could be swapped for $obj.

I can, of course, rename it to something else, $stdClass comes to mind. Agreed ?

Yes. I'm not precious about the names of the variables used in the rulesets - one minor task would be to minimise the variations of the variables names even more to reduce the size of line 5 to make it more reasonable. My thought was that we could even call them $string, $true, $obj, etc. and adjust the rest of the file to use the right type of variable where it makes sense. I didn't focus on that when I added VA, as I was just trying to get the tests to pass.

that still wouldn't take care of the fact that PHP 5 does not throw a parse error on line 7.

If PHP version check is needed for this, then that's fine. What's the reason for the parse error in PHP 5? Could we use another code snippet that is a syntax error for all PHP versions instead?

I do think the other changes related to the potential for 0 values in the lists should remain to make the script more resilient for future changes.

+1; I'd not seen any instance of those values until now.

[jrfnl]

@GaryJones I've updated the PR to address both issues in an alternative way.

The last two commits can be folded into the second commit realistically, but I've committed them separately for now to make reviewing the difference between the alternative solution and the original PR easier.

Would you like me to tidy up the commits before merge after you've reviewed ?

And to answer your questions:

What's the reason for the parse error in PHP 5?

Cannot re-assign $this

Could we use another code snippet that is a syntax error for all PHP versions instead?

Of course we can ;-)

My thought was that we could even call them $string, $true, $obj, etc. and adjust the rest of the file to use the right type of variable where it makes sense. I didn't focus on that when I added VA, as I was just trying to get the tests to pass.

I suggest we open an issue for that with low priority.

I'd not seen any instance of those values until now.

And they aren't there now anymore either, but having that defensive coding in place will save us scratching our heads again at a later point when it comes up again ;-)

[GaryJones]

Would you like me to tidy up the commits before merge after you've reviewed ?

Yes please, thanks!

[jrfnl]

@GaryJones Done.