Standards/AbstractSniffUnitTest: order test files using natural sort

[jrfnl]

When a sniff has multiple test case files, the AbstractSniffUnitTest::getTestFiles() sorts the file names before passing them off to be run.

While in most cases, the order of the test case files should not really matter, but in rare cases, like when testing that a property is being reset correctly between files, it does.

As things were, the sort() function without flags was being used, making the file order unnatural to work with ( file 11 would be run before file 2 - see: https://3v4l.org/VPO3Z ).

As the SORT_NATURAL flag is available since PHP 5.4, adding that flag will resolve this and will ensure the test case files are run in their natural order.

Optionally, the flag could be combined with the SORT_FLAG_CASE flag to make the sort case-insensitive.

[fredden]

Sorting 'naturally' makes sense over sorting strictly alphabetically, however I do wonder if we should be using shuffle($testFiles); here instead.

[jrfnl]

I do wonder if we should be using shuffle($testFiles); here instead.

No, we should not. See my comment about property reset testing above.

[fredden]

See my comment about property reset testing above.

Yes, I read that before reviewing the code. What I'm wondering about is if those tests should be rewritten so that the order in which they are run no longer matters. Anyway, this pull request seems like a good change as-is (hence the approval above).

[jrfnl]

See my comment about property reset testing above.

What I'm wondering about is if those tests should be rewritten so that the order in which they are run no longer matters.

That's not possible/desirable.

To give you a little more context/explain this a little more:
While most sniffs examine a token in place without additional context, some sniffs use the context of the current file or even take information seen in previous files into account.

Take for instance the Generic.Classes.DuplicateClassName sniff.
Test case file 1 declares a number of OO structures, which should also be flagged when seen as re-declared in file 2, 3, 4 etc.
Those tests wouldn't work properly if the files were run in a random order.

Another example would be a sniff examining code which may behave differently depending on a declare statement at the top of the file.
Now, there are three ways to examine such a file:

1. Sniff for the tokens you are looking for, then if you find a possible violation situation, walk the tokens above the current $stackPtr to find if there is a declare statement and if so, if it has an impact.
   This will be highly inefficient as the same token walking will have to be done multiple times, which depending on what is being sniffed for can run into the hundreds of times.
2. Walk through the complete file in one go and end the sniff with a return $phpcsFile->numTokens;.
   In a lot of cases, this will also be quite inefficient and will make the sniff slow.
3. Register whatever other tokens you are looking for + the T_DECLARE token.
   On a T_DECLARE token, examine the declare statement and save your findings to a property in the sniff + save the name of the current file to a property in the sniff.
   On the target token, examine the token + use the cached result from the declare statement examination (if any).
   In this case, the properties which hold the result of the declare statement examination + the file name will need to be reset on a new file, i.e. if ( $this->phpcsFile->getFilename() !== $this->current_file ) { /* reset the properties */ }
   This is the most efficient/fastest way for sniffing these kind of situations.
   For those sniffs, there should be tests to ensure that the property reset is in place and functioning correctly and testing this can only be done with test case files being run in a predefined order.

Now, you may say that sniffs should use the first or second way of sniffing, but depending on how large the files being examined are and how often the situation the sniff is looking for occurs, this can severely impact the runtime of PHPCS. I'm not talking seconds, this can make minutes of difference.

Does that make things a little clearer ?

[fredden]

Thanks very much for taking the time to explain this. Yes, this makes sense and helps me understand why it's important to keep the test files in a predefined order. The code comment simply says "Put them in order." which doesn't give any of the context that you've provided here. Both of these two test-cases make sense to me. I agree that having the test files in a predefined order is the most sensible approach here.

[jrfnl]

Closing as replaced by PHPCSStandards/PHP_CodeSniffer#55