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

Catch deprecations as part of test suite #85

Merged
merged 1 commit into from
Dec 19, 2021
Merged

Catch deprecations as part of test suite #85

merged 1 commit into from
Dec 19, 2021

Conversation

MasterOdin
Copy link
Contributor

@MasterOdin MasterOdin commented Dec 1, 2021

From #83 (comment), this PR enables failing tests if they throw deprecations. This required updating the CI pipeline as the shivammathur/setup-php@v2 does error_reporting=E_ALL | ~E_STRICT | ~E_DEPRECATION by default for PHP 8+ (ref: shivammathur/setup-php#450) as well as updating configuration for phpunit to convert deprecations into errors.

No existing tests fail from this change.

@willpower232
Copy link
Collaborator

I'm confused, shouldn't this have revealed a failure based on the other PR? Or is there a test missing?

In an ideal world, this PR would fail on tests and #83 would then make the tests pass but they haven't failed yet...

Or am I misunderstanding completely?

@MasterOdin
Copy link
Contributor Author

MasterOdin commented Dec 5, 2021

My view would be in an ideal world a PR with a bugfix or feature includes a test for that fix or feature. If the merged PR needs to be reverted for whatever reason, so to could be the test. This also keeps the default branch consistently "green" as a primary goal.

This PR is just making it so that that test (and future tests) would probably validate that a deprecation warning is not being thrown.

I'm confused, shouldn't this have revealed a failure based on the other PR? Or is there a test missing?

There are currently no tests that covers setting the $issuer to null, and so catching the deprecations would not cause any "new" failures. As part of #83, a test should be added for that particular case though.

@willpower232 willpower232 merged commit 4203749 into RobThree:master Dec 19, 2021
@willpower232
Copy link
Collaborator

Test added in 4f1543f and deprecation finally detected with this PR merged

@MasterOdin MasterOdin deleted the mpeveler/chore-deprecations branch December 19, 2021 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants