-
Notifications
You must be signed in to change notification settings - Fork 21
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
Orca 559 : Compatibility with PHPunit 10 #420
Conversation
secretsayan
commented
Aug 3, 2023
•
edited
Loading
edited
- Coverage was enabled for all tests, as tests were failing due to a warning saying "coverage driver is not available" , may be we should resolve "failonwarning" settings of phpunit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in general, with a few suggestions. (I hope there was a script to change all those data provider signatures to static for you!)
As to the removal of PHPLOC, that's a functional change that should have a separate PR. No one looking at this PR would say, "Hmm, Compatibility with PHPunit 10. I'd better make sure it didn't change the CLI API." Also, when you announce the change, you'll want to link to a PR where it and it only is discussed.
tests/Domain/Composer/Version/DrupalCoreVersionResolverTest.php
Outdated
Show resolved
Hide resolved
@TravisCarden : Please review this PR, I have deprected |
a4c8ff0
to
ffee917
Compare
@TravisCarden Please review this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm gonna start being a little rougher on my reviews, @secretsayan. 😉 Please take this one back and look it over again before I review it. After a quick skim, I see some fairly basic oversights.
c288f2c
to
a8b0f26
Compare
This reverts commit ea3bc73.
Co-authored-by: Travis Carden <[email protected]>
This reverts commit 44de03a.
This reverts commit ad98f0c.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, @secretsayan! I left one question that you can mark resolved or address based on the answer. Merge at your pleasure. 🙂