Split the errors, adding the *CloserSameLine ones

[stronk7]

Some standards may want to have different rules when the array closer is in the same line than the last element.

When that happens, the new *CloserSameLine errors are emitted, allowing to disable them via ruleset.

For testing, a couple of cases have been added to CommaAfterLastUnitTest.1.inc and then, the fixtures have been 100% duplicated into CommaAfterLastUnitTest.3.inc that runs with the *CloserSameLine errors disabled.

A simple diff shows the 8 cases in which the outcome is different, as expected.

Fixes #283.

[stronk7]

OT, sharing because it happened to me... I just found that something in composer is not installing things properly, neither vendor/bin/phpunit or composer test (and others) seem to be working by default. Had to remove the lock file, remove the platform constraint and other custom installations to get it working locally.

[jrfnl]

@stronk7 Thanks for the PR, I'll try to review it over the next few days.

OT, sharing because it happened to me... I just found that something in composer is not installing things properly, neither vendor/bin/phpunit or composer test (and others) seem to be working by default. Had to remove the lock file, remove the platform constraint and other custom installations to get it working locally.

Just tried myself and all seems fine, so I'm not sure what happened for you.
Removing the lock file and (re-)running composer update (on PHP 7.4 or lower) is never a bad idea though if you cloned a while back.
There is no committed composer.lock file, so that is normal.

As for the "platform constraint": the repo doesn't have one, so I don't know what you mean.

The only real (known) problem with the tests is that you need to run them locally on PHP 7.4 or lower as otherwise you run into issues with PHPUnit 7.5. There are work-arounds for that, see: https://github.com/PHPCSStandards/PHPCSExtra/blob/develop/.github/workflows/quicktest.yml , but the simple solution is to just run on PHP 7.4.
This is a limitation in the PHPCS native test suite which is used for the tests and I'm trying to get that limitation lifted.

[jrfnl]

@stronk7 I've just had a quick glance at the PR and first thing I noticed is that the new error code is not just reserved for multi-line arrays.
This will need to be fixed, as differentiating the error code doesn't make sense for a single line array where the closer will always be on the same line and it would basically mean the FoundSingleLine and MissingSingleLine codes would be replaced with the new codes which is a much bigger and unnecessary breaking change.

[jrfnl]

P.S.: no need to worry about the code coverage builds failing - this is a known issue and I'm working with Coveralls to fix that.

[stronk7]

Thanks for the composer comments and tricks… I’ll try again tomorrow from scratch. Maybe I had some old lock file around and that caused the troubles…

Regarding applying it only to multilines, cannot but agree. As commented in the issue, it really doesn’t make sense for singles. I will amend the PR tomorrow, thanks!

[stronk7]

Patch updated to only apply the "CloserSameLine" error code suffix to multi lines (with tests amended to ensure that's what it's being done).

Ciao :-)

[jrfnl]

@stronk7 Hi Eloy, thank you for fixing that up.

I've now done a proper review of the PR and the principle of the fix is now correct 👍🏻 .
I've left some nitpicky comments inline to tidy that up a little still, but that doesn't affect the actual functional change, which is good.

As for the tests...
While I appreciate how much attention you've given to the tests, the way this is done now, means the diff is much bigger than it should be, what with all test lines being renumbered. This also makes it much harder than it should be to review the PR.

As a rule of thumb: always put new tests at the bottom of a test case file (or in a new file if so warranted) with a comment explaining what those tests are related to.
Do not touch or cause to re-number the pre-existing tests.

In all honesty, the 3 test case file is redundant, as it isn't testing anything new in the PHPCSExtra repo. It is testing whether PHPCS handles the disable/ignore annotations correctly, but that is something which should be (and is) tested in the PHPCS repo.

So, in short:

• Please remove the 3 test case file.
• Please remove the comment at the top of the 1 test case file.
• Please move the new tests in the 1 test case file to the bottom of the file (just above the reset) and add appropriate phpcs:set comments for each situation and add a comment to describe why these tests were added and what is specifically being tested with them. Include the issue number in the comment as a reference.

As for the actual tests, these are only testing the "unhappy" path (= the path which throws an error).
I'm missing tests with multi-line arrays with the closer on the same line as the last content, which don't trigger an error, both for the forbid, as well as the enforce case.

And for completeness of testing the actual change, you may want to also add single line test code just to demonstrate that those aren't affected, but I'm fine with leaving those out as well, as this can also be confirmed via the pre-existing tests.

Sorry to be a pain. Please let me know if you have questions.

[jrfnl]

P.S.: if you rebase the PR on the current develop, the code coverage builds should start working ;-)

[stronk7]

Sorry to be a pain. Please let me know if you have questions.

No worries, I'm used, LOL :-)

Thanks for the review, I'll go towards all the the test changes and suggestions later today.

[stronk7]

Ok, I think I've followed all your suggestions and the new candidate PR follows all them.

Only point that I'm not 100% convinced is the one about not covering with tests what happens when the CloserSameLine error is disabled. While I agree that it's phpcs itself the one to make the disabling to work... it's the only way we have to ensure that we are setting the CloserSameLine new error properly. I mean, current tests don't cover the new code introduced in the Sniff. At all (in fact they will pass exactly the same without the Sniff changes).

Note I don't mind much, because for sure we are going to add some tests covering that in our standard, but I think that it should be tested here because, as said, it's not covering the new real code at all.

Ciao :-)

[stronk7]

Adding on my previous comment... what I'd add to the tests is this (I think it's the minimum patch I've been able to create):

stronk7@f971d5b

You can see that it doesn't change the tests outcome (all the new lines are ok, because we are ignoring the CloserSameLineerror) but, that way, we are getting covered the new Sniff code.

For your consideration, I'm happy to fixup this PR with that commit or to leave it apart (and to add a similar testing to our standard, ensuring that the new error codes work as expected).

Ciao :-)

[jrfnl]

@stronk7

Only point that I'm not 100% convinced is the one about not covering with tests what happens when the CloserSameLine error is disabled. While I agree that it's phpcs itself the one to make the disabling to work... it's the only way we have to ensure that we are setting the CloserSameLine new error properly. I mean, current tests don't cover the new code introduced in the Sniff. At all (in fact they will pass exactly the same without the Sniff changes).

Note I don't mind much, because for sure we are going to add some tests covering that in our standard, but I think that it should be tested here because, as said, it's not covering the new real code at all.

Adding on my previous comment... what I'd add to the tests is this (I think it's the minimum patch I've been able to create):

stronk7@f971d5b

You can see that it doesn't change the tests outcome (all the new lines are ok, because we are ignoring the CloserSameLineerror) but, that way, we are getting covered the new Sniff code.

Okay, so let's split this up:

1. The actual code does get hit via these tests (also see the code coverage), so the code does actually get tested.
2. The tests wouldn't fail without this change.

I agree that 2 is problematic, and that's inherent to the PHPCS native test framework, which is being used as a base for these tests.
It's on my never-ending to-do list to create an alternative test setup for external standards, which would allow for testing that the error codes are as expected (and more). Also see: PHPCSStandards/PHPCSUtils#72

I'd be okay with you adding the extra tests you propose. I think only one for each type (missing/found) would be fine and that those can be in the same // phpcs:set... blocks as the tests you've got in place now.

[stronk7]

I'd be okay with you adding the extra tests you propose. I think only one for each type (missing/found) would be fine and that those can be in the same // phpcs:set... blocks as the tests you've got in place now.

Ok, will amend the commit to incorporate ONE missing and ONE found case to the 2 "new sections" fixtures introduced here.

OT: Since we started to use phpcs long ago... one of the first things we did is to switch to a "richer" testing alternative. It supports the normal line => num_errors thing of phpcs and more, with both the error message and the error code being available for substring assertions. Just sharing in case it gives some idea to you (in fact I think we shared about it @ phpcs ages ago).

[jrfnl]

I'd be okay with you adding the extra tests you propose. I think only one for each type (missing/found) would be fine and that those can be in the same // phpcs:set... blocks as the tests you've got in place now.

Ok, will amend the commit to incorporate ONE missing and ONE found case to the 2 "new sections" fixtures introduced here.

👍🏻

OT: Since we started to use phpcs long ago... one of the first things we did is to switch to a "richer" testing alternative. It supports the normal line => num_errors thing of phpcs and more, with both the error message and the error code being available for substring assertions. Just sharing in case it gives some idea to you (in fact I think we shared about it @ phpcs ages ago).

Looks interesting, but not quite what I had in mind. We use a custom test case in PHPCompatibility as well.

Basically, what I'm looking to create would be a "best of these worlds" situation, where it would give sniff writers the flexibility to test what they want to test, while also making the test suites straight-forward to maintain. Keep en eye on PHPCSUtils or subscribe to the issue if you'd like to stay informed.

[stronk7]

Patch amended, by adding the 2 agreed new cases (using phpcs:ignore).

Keep en eye on PHPCSUtils or subscribe to the issue if you'd like to stay informed.
👍

[jrfnl]

All good! Thanks for contributing and for bearing with me to get this to where it is now.