Generic/DisallowLongArraySyntax: improve code coverage

[rodrigoprimo]

Description

This PR improves code coverage for the Generic.Arrays.DisallowLongArraySyntax sniff. It doesn't add any new tests as I couldn't think of any relevant missing tests. But it does remove an unreachable condition.

There are a few tests that I'm not sure if they belong to the sniff or not:

https://github.com/rodrigoprimo/PHP_CodeSniffer/blob/8f31c91c4a520d52ec97d819a7ce9d7da5b3fb40/src/Standards/Generic/Tests/Arrays/DisallowLongArraySyntaxUnitTest.1.inc#L16-L22

They were added in ee0f59c and it seems this commit is about a change in the PHP Tokenizer, so I wonder if those tests should be moved to the PHP Tokenizer tests?

Related issues/external references

Part of #146

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
  • This change is only breaking for integrators, not for external standards or end-users.
• Documentation improvement

PR checklist

• I have checked there is no other PR open for the same change.
• I have read the Contribution Guidelines.
• I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
• I have added tests to cover my changes.
• I have verified that the code complies with the projects coding standards.
• [Required for new sniffs] I have added XML documentation for the sniff.

[jrfnl]

@rodrigoprimo Thanks for another PR in this series.

It doesn't add any new tests as I couldn't think of any relevant missing tests.

I can 😎

1. In the second commit, you simplify a condition, but only one part of the condition has a test (missing open parenthesis). The second part of the condition (missing close parenthesis) is currently untested.

2. Another test which appears to be missing is for something which looks like an array, but isn't:

   $obj->array( 1, 2, 3 );
   class Foo {
       function array() {}
   }

   Neither of the above should trigger the sniff as the Tokenizer should have retokenized the array keyword to T_STRING, but I do still think it's valid to have these as tests for this sniff as an extra safeguard and to document that the sniff will leave these alone.

3. And what about having a test (possibly via updating one of the existing tests) which uses the array keyword in non-lowercase ?

While the tests I suggest in [2] and [3] don't test anything new, some redundancy is not necessarily a bad thing.

But it does remove an unreachable condition.

Good call. That condition is likely an artifact from when the Tokenizer didn't handle certain situations yet.

There are a few tests that I'm not sure if they belong to the sniff or not:

... it seems this commit is about a change in the PHP Tokenizer, so I wonder if those tests should be moved to the PHP Tokenizer tests?

I think the tests on line 16 and 18 (and on line 25 for that matter) have a right to be included in this test case file as they use the array keyword, even though it is not used for an array declaration.

I agree with you, though, that the tests on line 20 and 22 should probably be moved to the Tokenizer tests (if they aren't covered there already).

You will find test artifacts like that in more test case files as prior to PHPCS 3.5.0, there were no dedicated tests for the Tokenizer at all. Until that time, the Tokenizer was only ever tested via the sniff tests.

[rodrigoprimo]

Thanks for the review, @jrfnl!

In the second commit, you simplify a condition, but only one part of the condition has a test (missing open parenthesis). The second part of the condition (missing close parenthesis) is currently untested.

I believe the test in DisallowLongArraySyntaxUnitTest.2.inc covers the missing close parenthesis part of the condition, doesn't it?

Another test which appears to be missing is for something which looks like an array, but isn't

And what about having a test (possibly via updating one of the existing tests) which uses the array keyword in non-lowercase ?

I added another commit with the tests that you suggested and changed one of the existing tests to use the uppercase version of the array keyword.

I agree with you, though, that the tests on line 20 and 22 should probably be moved to the Tokenizer tests (if they aren't covered there already).

As we discussed via chat, I will work on a separate PR to add dedicated tokenizer tests for parameter types and return types. In this PR I will remove the tests on lines 20 and 22.

[jrfnl]

In the second commit, you simplify a condition, but only one part of the condition has a test (missing open parenthesis). The second part of the condition (missing close parenthesis) is currently untested.

I believe the test in DisallowLongArraySyntaxUnitTest.2.inc covers the missing close parenthesis part of the condition, doesn't it?

Checked this out a little deeper and yes, you are right, the 2 file covers the missing close parenthesis. However, the DisallowLongArraySyntaxUnitTest.3.inc file isn't testing anything as the array keyword is not seen as a T_ARRAY token. If it were, it should trigger the non-fixable error, but it doesn't.

The test should be changed like the below diff to actually have value:

-$var = array;
+$var = array

[rodrigoprimo]

Good catch. I had seen that the array keyword was not tokenized as T_ARRAY, but wrongly assumed that the same would happen without the semicolon. I decided to leave the semicolon as the original test contained it.

This is fixed now. Could you please take another look?

[jrfnl]

This is fixed now. Could you please take another look?

That test should now trigger a non-fixable error, but that expectation is not set in the test file, so the build would fail.

[rodrigoprimo]

Apologies, I forgot to include the changes to DisallowLongArraySyntaxUnitTest.php in the commit that I made yesterday. The commit is now amended and the tests are passing.

[jrfnl]

Thanks for fixing that. All good now. Thanks @rodrigoprimo !

I will rebase once I've merged the other PR and then merge this once the build has passed.