Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Increase test coverage for --sniffs and --exclude options #474

Merged

Conversation

fredden
Copy link
Member

@fredden fredden commented Apr 28, 2024

Description

While working on #344, it was noted that there are no existing tests for the code being changed. This pull request aims to add some tests to cover the existing behaviour.

Suggested changelog entry

  • Improve test coverage of internal functionality

Related issues/external references

Related to #344

Types of changes

  • Chore - no functional changes, only code coverage

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.

@fredden fredden force-pushed the feature/tests/config.sniffs-exclude branch from 89f3673 to 6bba777 Compare April 30, 2024 17:54
@fredden fredden requested a review from jrfnl April 30, 2024 18:32
Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fredden Thank you for this PR. Really happy to see tests for this part of the code base being added.

I've gone through the file with a fine toothcomb and left a number of nitpick comments, but also some functional questions.

Hope this helps for the next iteration.

tests/Core/Config/SniffListTest.php Outdated Show resolved Hide resolved
tests/Core/Config/SniffListTest.php Outdated Show resolved Hide resolved
tests/Core/Config/SniffListTest.php Outdated Show resolved Hide resolved
tests/Core/Config/SniffListTest.php Outdated Show resolved Hide resolved
tests/Core/Config/SniffListTest.php Outdated Show resolved Hide resolved
tests/Core/Config/SniffListTest.php Outdated Show resolved Hide resolved
tests/Core/Config/SniffListTest.php Outdated Show resolved Hide resolved
tests/Core/Config/SniffListTest.php Outdated Show resolved Hide resolved
tests/Core/Config/SniffListTest.php Outdated Show resolved Hide resolved
tests/Core/Config/SniffListTest.php Outdated Show resolved Hide resolved
@fredden
Copy link
Member Author

fredden commented May 6, 2024

@jrfnl, I think this is ready for another review now. When we're happier with the final delta, I can rewrite the history so there are fewer commits before this is merged in.

Now that we have tests asserting the result of these arguments and not just
validation of their input.
Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fredden Thanks for the updates, looking much better!

The @return array<...> tags are outdated now you've added the test names, so they need an update again.

Other than that, just one remark to go and this is ready for merge.

tests/Core/Config/SniffsExcludeArgsTest.php Outdated Show resolved Hide resolved
tests/Core/Config/SniffsExcludeArgsTest.php Outdated Show resolved Hide resolved
tests/Core/Config/SniffsExcludeArgsTest.php Outdated Show resolved Hide resolved
tests/Core/Config/SniffsExcludeArgsTest.php Outdated Show resolved Hide resolved
tests/Core/Config/SniffsExcludeArgsTest.php Outdated Show resolved Hide resolved
fredden and others added 7 commits May 13, 2024 22:12
Co-authored-by: Juliette <663378+jrfnl@users.noreply.github.com>
Co-authored-by: Juliette <663378+jrfnl@users.noreply.github.com>
Co-authored-by: Juliette <663378+jrfnl@users.noreply.github.com>
Fixes copy/paste error

Co-authored-by: Juliette <663378+jrfnl@users.noreply.github.com>
@jrfnl jrfnl added this to the 3.10.0 milestone May 14, 2024
Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fredden Thanks for your work on this Dan! All good now as far as I'm concerned.

Re: your remark about tidying up the branch- what did you have in mind ? I could squash-merge the PR, unless you have a reason why you'd prefer otherwise ?

@fredden
Copy link
Member Author

fredden commented May 14, 2024

🎉 Yes. A squash-merge via GitHub is fine with me.
Thanks for your coaching here too @jrfnl.

@jrfnl jrfnl merged commit 05f8b21 into PHPCSStandards:master May 14, 2024
40 checks passed
jrfnl pushed a commit that referenced this pull request May 14, 2024
This PR adds tests covering the logic for handling the `--sniffs=...` and `--exclude=...` CLI options, which both take a comma-separated list of sniff codes and will error on anything which is not a sniff code.
@fredden fredden deleted the feature/tests/config.sniffs-exclude branch May 14, 2024 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants