Generic/DocComment: improve code coverage

[rodrigoprimo]

Description

This PR improves the code coverage for the Generic.Commenting.DocComment sniff.

See commit messages for more details on the reasoning behind some of the changes, including one that I'm not sure if I took the best approach (handling errors in different lines for the JS and PHP test case files).

While working on this PR, I noticed that PHPCS considers /**/ a doc comment, but php.net states that "doc comments start with /**, followed by whitespace" (https://www.php.net/manual/en/reflectionclass.getdoccomment.php). I'm not sure why PHPCS doesn't require the whitespace after /**, but this seems to be intentional. Here is the commit that introduced this change: 8c64ded. I'm sharing this here in case it is something that we want to further investigate.

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]

While working on this PR, I noticed that PHPCS considers /**/ a doc comment, but php.net states that "doc comments start with /**, followed by whitespace" (https://www.php.net/manual/en/reflectionclass.getdoccomment.php). I'm not sure why PHPCS doesn't require the whitespace after /**, but this seems to be intentional. Here is the commit that introduced this change: 8c64ded. I'm sharing this here in case it is something that we want to further investigate.

@rodrigoprimo Good observation, but this is exactly as it should be. PHPCS needs to be able to flag these kind of user errors and should therefore tokenize it in a way that allows us to do so.

[jrfnl]

See commit messages for more details on the reasoning behind some of the changes, including one that I'm not sure if I took the best approach (handling errors in different lines for the JS and PHP test case files).

For clarity I'm copying the commit message referred to above here:

I opted to separate the array containing the list of errors for
DocCommentUnitTest.1.inc and DocCommentUnitTest.js as the files had
already diverged even before this commit and I believe it will be easier
to just remove the switch case for JS when JS support is dropped.

@rodrigoprimo While I understand your reasoning, I don't think the reasoning is correct.

1. At this time, in the 3.x branch JS is still supported, so that same error code should also be tested for JS (if possible).
2. Splitting it up is not going to make dropping JS easier as JS has already been dropped in the 4.x branch, so this would now make the cherrypick to the 4.x branch more complicated instead of easier.

As for:

the files had already diverged

That raises some questions. I mean, I would expect the files to be different as JS is a different language than PHP, but the error list is the same, so how have they diverged ?

[rodrigoprimo]

At this time, in the 3.x branch JS is still supported, so that same error code should also be tested for JS (if possible).
(...)
That raises some questions. I mean, I would expect the files to be different as JS is a different language than PHP, but the error list is the same, so how have they diverged ?

What I meant to say when I said that they diverged is that there are two comments that were added to the PHP test case file but not to the JS test case file:

https://github.com/PHPCSStandards/PHP_CodeSniffer/blob/master/src/Standards/Generic/Tests/Commenting/DocCommentUnitTest.inc#L253-L262

That led me to believe that I should add the new tests only to the PHP file.

Splitting it up is not going to make dropping JS easier as JS has already been dropped in the 4.x branch, so this would now make the cherrypick to the 4.x branch more complicated instead of easier.

👍

I pushed new commits copying the missing tests from the main PHP test case file to the JS test case file and undoing the change that I made to DocCommentUnitTest.php. Can you please check this PR again?

[jrfnl]

What I meant to say when I said that they diverged is that there are two comments that were added to the PHP test case file but not to the JS test case file

Ah. That was an oversight from my side as those tests didn't (and shouldn't) yield any errors. I would have caught it when adding them if they did yield errors.

[jrfnl]

@rodrigoprimo Thanks for making those additional changes. All looks good now.

I will rebase the PR and squash some of the commits together now (without making changes) to get this ready for commit.

[jrfnl]

Ended up making some tiny tweaks:

• Parse error test: kept the "Must be last test without new line." comment and removed the new line.
  If this was explicitly marked as such, there was a reason for it. The sniff may handle it correctly now, but likely didn't in the past.
  As there is no reason to change the test, better not to.
• Proper capitalization and punctuation for the "// Tests to check handling empty doc comments." comment.