Tests: parse error tests for sniffs should be in their own file

[jrfnl]

Basic premise

Sniffs at times need to gracefully handle parse errors in the code under scan and often contain defensive coding for this.

This code should be covered by tests.

Current situation

A sniff may have multiple blocks of logic to handle parse errors, but oftentimes only has one test case file.

This is not so much of a problem with compile errors, but for parse errors, it generally means that the test case file only contains one test with a parse error at the end of the test case file, which also means that if the sniff contains more than one block of logic related to parse errors, only one of those code blocks is covered by tests.

This also makes it more fiddly for (new) contributors to know where to add a test when they update a sniff as they need to be careful to make sure that their new test is added above the parse error test, instead of at the very end of the file.

Lastly, if the parse error would be one where the sniff would throw an error, it also means that the line numbers for the test expectations need to be updated every time an extra test is added (above the parse error test).

Desired situation

• Parse error related tests should be in their own test case file.
• Tests files containing a parse error test should only contain that test and no other and be clearly annotated as a parse error test.

Example of tests where this is done correctly

The tests for the Generic.ControlStructures.InlineControlStructure sniff: https://github.com/PHPCSStandards/PHP_CodeSniffer/tree/master/src/Standards/Generic/Tests/ControlStructures

• The InlineControlStructureUnitTest.1.inc does not contain a parse error test at the end.
• There are multiple additional test case files which each contain one test for a specific parse error the sniff handles. Each of those tests is also clearly annotated as being a parse error test.

Note: this does not mean that all parse error related code blocks in the sniff are comprehensively tested (yet). This is just a good example of how the tests for parse errors should be structured.

Example of a test where this is not done correctly

• PHP_CodeSniffer/src/Standards/Squiz/Tests/Objects/ObjectInstantiationUnitTest.inc

  Lines 51 to 52 in e844821

  	// Intentional parse error. This must be the last test in the file.
  	function new

Future scope

It would be good to prevent accidental parse errors from creeping into the test case files as in that case, the test may not sufficient safeguard the working of the sniff.

Once all parse error related tests are in their own files, a linting task could be added to CI to run over the non-parse error test case files to safeguard this.

Tasks

Typical ways to approach this task:

• Look for tests in the test suites for the sniffs which are already annotated as parse error/live coding tests and check if the test is in its own file. If not, fix it.
• Run a linter over test case files to see if they contain parse error tests.
  When parse errors are found,:
  • When the test is not marked as a parse error test, it may be an accidental parse error, which should be fixed.
  • Or it may be an intentional parse error missing the annotation, in which case the test should be moved to its own file.
  • Git blame can often help to determine whether a parse error in the test case file is intentional or accidental.

Some practical guidelines:

1. If there is only one test case file for the sniff, rename the file from SniffNameUnitTest.inc to SniffNameUnitTest.1.inc (same for potential .fixed test case files) and update the SniffNameUnitTest.php file to expect the $testFile parameter and use a switch statement to return the right array.
   Commit this in a separate commit. That way the chance that git will recognize this as a renamed file will be largest, which means history checks for the test case file won't be broken.
2. Move the parse error test to a new SniffNameUnitTest.2.inc file, if applicable do the same for the .fixed file, and if necessary, update the expectations in the SniffNameUnitTest.php file.

This check should be executed for the complete all sniff tests.

PRs related to this task should preferably only touch the tests for one sniff per PR.