tests: Skip cases incompatible with PHP 8#822
tests: Skip cases incompatible with PHP 8#822jtojnar wants to merge 1 commit intosimplepie:masterfrom
Conversation
jrfnl
left a comment
There was a problem hiding this comment.
@jtojnar I agree these tests will need to be skipped when the tests start running on PHPUnit 10.
However...
- SimplePie is not using PHPUnit 10 (yet).
The current warning doesn't do any harm. - The test would now be skipped on PHP >= 8.0, while on PHP 8.0 (which would be using PHPUnit 9.x even when PHPUnit 10 would start to be allowed), it can still run.
I.e. it is now being skipped on more PHP versions then needed. - The current change doesn't include a "skip reason", which is recommended to allow people running the tests in
verbosemode to quickly see why tests are being skipped.
I'd recommend using the @requires PHPUnit < 10 docblock annotation instead.
Aternatively the PHPUnit 10 attribute could be used: #[RequiresPhpunit('< 10.0.0')]. though that may run into "Unknown class" errors ?
Whether the annotation or the attribute is used, in both cases a "skip reason" will be shown and the test should still run on PHP 8.0.
As a side note: there is no PHPUnit 10.x compatible release (yet) for the PHPUnit Polyfills (used under the hood to allow the tests to run on multiple PHP/PHPunit versions).
A new release which supports PHPUnit 10.x is being worked on and the roadmap has been published and is open for comments: Yoast/PHPUnit-Polyfills#95
1ded14e to
34dc7ef
Compare
instead of running them and expecting an error.
Arguably, it is semantically cleaner to avoid testing a behaviour that is defined by PHP itself rather than anything SimplePie is doing.
As a bonus, it gets rid of the following warning:
Expecting E_ERROR and E_USER_ERROR is deprecated and will no longer be possible in PHPUnit 10.
34dc7ef to
f99b28d
Compare
It is a bit annoying, though the primary reason I went for this change is that it felt weird essentially testing a behaviour that is defined by PHP itself rather than anything SimplePie is doing.
I am not sure why it could run on PHP 8.0, it looks like the original version check was correct according to https://php.watch/versions/8.0/non-static-static-call-fatal-error and I just inverted it.
Thanks. Added.
I would probably wait for PHPUnit 10.x compatible release of PHPUnit Polyfills. |
The Polyfills are forward-compatible, not backward-compatible, so functionality removed from PHPUnit (ability to expect PHP errors) will also be removed from the Polyfills, so I don't think that will make a difference. Also see the Roadmap. |
|
Oh, that makes sense. I assumed it was more like Symfony’s PHPUnit Bridge. I like the Polyfills better then. If the functionality will be removed, would not |
Depends on whether or not a Helper class for transitioning will be added to the Polyfills. The trait will definitely be removed, but I haven't seen anyone give an opinion yet on whether or not they think having a Helper class (to selectively use) would be helpful. My own biggest concern with this change in PHPUnit is that packages can no longer deprecate functionality + test the deprecation notice. I think that will be (is) the biggest point of friction. |
|
We are now using PHPUnit 9+10 and have removed the dependency to Yoast\PHPUnitPolyfills, see #848. |
instead of running them and expecting an error.
Arguably, it is semantically cleaner to avoid testing a behaviour that is defined by PHP itself rather than anything SimplePie is doing.
As a bonus, it gets rid of the following warning: