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

Tests: Add unit tests for Common::prepareForOutput() #663

Merged
merged 2 commits into from
Nov 13, 2024

Conversation

MatmaRex
Copy link
Contributor

This function has different behavior on Windows and on other systems, therefore add two tests, only one of which runs.

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.

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.

@MatmaRex This is great ! Thank you for adding these tests.

I've left a few small comments to have a look at.

Other than that, maybe there should also be a test case which doesn't do any replacements, not on Windows, nor on Linux. Something like this maybe ?

            'No replacements' => [
                'content'     => 'nothing-should-be-replaced',
                'exclude'     => [],
                'expected'    => 'nothing-should-be-replaced',
                'expectedWin' => 'nothing-should-be-replaced',
            ],

If you rebase the PR, you can remove the @author tag.

Once that's done, I'll add an extra commit to the PR to enable running the tests on Windows in GH Actions.

tests/Core/Util/Common/PrepareForOutputTest.php Outdated Show resolved Hide resolved
tests/Core/Util/Common/PrepareForOutputTest.php Outdated Show resolved Hide resolved
tests/Core/Util/Common/PrepareForOutputTest.php Outdated Show resolved Hide resolved
@MatmaRex MatmaRex force-pushed the prepareForOutput-tests branch 2 times, most recently from 1d8aa22 to 8bee9c4 Compare November 12, 2024 23:58
This function has different behavior on Windows and on other systems,
therefore add two tests, only one of which runs.
@MatmaRex MatmaRex force-pushed the prepareForOutput-tests branch from 8bee9c4 to a220c80 Compare November 13, 2024 00:04
@MatmaRex
Copy link
Contributor Author

Thank you for the review, addressed all comments.

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.

@MatmaRex Thank you for making those updates! I've now added that extra commit to enable running the tests on Windows. 🤞🏻

Once the build has run (and passed), I'll merge this PR.

@jrfnl jrfnl added this to the 3.11.1 milestone Nov 13, 2024
@jrfnl jrfnl force-pushed the prepareForOutput-tests branch from be92a4b to f7dcac5 Compare November 13, 2024 10:10
@jrfnl
Copy link
Member

jrfnl commented Nov 13, 2024

Sorry for the commit noise, just tweaking a step to (hopefully) get code coverage running on Windows too.

@jrfnl jrfnl force-pushed the prepareForOutput-tests branch from b939bb4 to 7bdab2b Compare November 13, 2024 14:27
For the `quicktest` and the `coverage` jobs, only select tests which are marked with `@group Windows` will be run.

For the "normal " `test` job, all tests will now be run on Windows too.
@jrfnl
Copy link
Member

jrfnl commented Nov 13, 2024

Again, sorry for the commit noise. Looks like I've finally got it working correctly. I'm going to squash the CI commits now. Once the build has finished once again, I'll merge this.

@jrfnl jrfnl force-pushed the prepareForOutput-tests branch from 170a79a to 755f0bc Compare November 13, 2024 15:15
@jrfnl jrfnl merged commit 0194e46 into PHPCSStandards:master Nov 13, 2024
59 checks passed
@MatmaRex
Copy link
Contributor Author

Thanks!

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.

2 participants