search

PHPCSStandards / PHP_CodeSniffer

PHP_CodeSniffer tokenizes PHP files and detects violations of a defined set of coding standards.
BSD 3-Clause "New" or "Revised" License
856 stars 53 forks source link

Generic/DisallowLongArraySyntax: improve code coverage #455

Closed rodrigoprimo closed 3 months ago

rodrigoprimo commented 4 months ago

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 https://github.com/PHPCSStandards/PHP_CodeSniffer/issues/146

Types of changes

PR checklist

jrfnl commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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.