-
Notifications
You must be signed in to change notification settings - Fork 13
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
PHPUnit 11 | ExpectUserDeprecationtrait: polyfill the TestCase::expectUserDeprecation*() methods #200
Merged
hellofromtonya
merged 1 commit into
3.x
from
feature/3.x/new-expectuserdeprecation-polyfill-trait
Sep 6, 2024
Merged
PHPUnit 11 | ExpectUserDeprecationtrait: polyfill the TestCase::expectUserDeprecation*() methods #200
hellofromtonya
merged 1 commit into
3.x
from
feature/3.x/new-expectuserdeprecation-polyfill-trait
Sep 6, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hellofromtonya
approved these changes
Sep 6, 2024
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.
LGTM 👍
…tUserDeprecation*() methods PHPUnit 11.0.0 introduces the new `TestCase::expectUserDeprecationMessage()` and `TestCase::expectUserDeprecationMessageMatches()` methods. These methods can largely be seen as replacements for the `TestCase::expectDeprecationMessage()` and `TestCase::expectDeprecationMessageMatches()` methods which were removed in PHPUnit 10.0, though there are significant differences between the implementation details of the old vs the new methods. As PHPUnit 10 does not have any even semi-equivalent method(s) available, the challenge was to polyfill these methods for PHPUnit 10. And as these methods are expectation methods, not assertions, this is even more challenging as the information to evaluate a pass/fail is not available at the time of the method call, but only once the rest of the test code has run. To do so, the following five options were considered: **Option 1: evaluate pass/fail in an "assertPostConditions()" method** While possible, doing so would make the use of the `Yoast\PHPUnitPolyfills\TestCases\TestCase` base class required. It would also mean deprecating/removing the annotation-based `Yoast\PHPUnitPolyfills\TestCases\XTestCase` class as there is no annotation available to mark a method as an "assertPostConditions()" method, so the functionality could not work with the `XTestCase` class. As a knock-on effect of making the use of the Polyfilled `TestCase` a requirement, it would also mean that using of the individual Polyfills as stand-alone traits should be deprecated/no longer be supported. This chain of consequences was deemed undesirable as it breaks the premise of the Polyfills being a "drop in" helper library. **Option 2: use Events to evaluate pass/fail** While possible, doing so would mean that the event listeners would need to be "hooked in" from the PHPUnit config file. This would mean that every project using the Polyfills and wanting to use the `expectUserDeprecation*()` methods would need to make changes to their PHPUnit configuration file. Additionally, adding the event listeners would make a configuration file incompatible with PHPUnit <= 9 and the event listener should not be hooked in for PHPUnit 11, in which the functionality exists in PHPUnit natively. In other words, it would mean that end-users would now need three (3) different PHPUnit configuration files, one for PHPUnit <=9, one for PHPUnit 10 and one for PHPUnit 11. Again, this was deemed undesirable as it breaks the premise of the Polyfills being a "drop in" helper library. **Option 3: polyfill, but don't support PHPUnit 10 in the polyfill** This would basically mean that any test using the `expectUserDeprecation*()` polyfill would be marked as skipped on PHPUnit 10. This is not a transparent action and the impact of this on tests was deemed too high to be acceptable. **Option 4: don't polyfill** It should be obvious that this is the least desirable option as this would mean the polyfills wouldn't polyfill (at least for these methods). **Option 5: polyfill, but drop support for PHPUnit 10 completely** This means polyfilling for PHPUnit <= 9 by dropping through to the "old" functions and not supporting installation of the Polyfills 3.x versions in combination with PHPUnit 10. In practical terms, the net effect of this is that tests on PHP 8.1 would run on PHPUnit 9 instead of PHPUnit 10. Other than that, there is no downside. Having considered these options carefully, option 5 was deemed most appropriate and most in line with the simple elegance of the functionality offered by the PHPUnit Polyfills until now. Final note: when falling through to the "old" methods on PHPUnit 9.5.x, PHPUnit native deprecation notices will be shown (but won't fail a build). Unfortunately, this is unavoidable as the `@` operator is not respected and cannot silence these deprecation notices. Having said that, as long as the polyfilled methods are used in the actual test suite, these deprecation notices can be safely ignored anyway. This commit implements option 5: * Adds two traits with the same name. One to polyfill the methods when not available in PHPUnit. The other - an empty trait - to allow for `use`-ing the trait in PHPUnit versions in which the methods are already natively available. * Adds logic to the custom autoloader which will load the correct trait depending on the PHPUnit version used. * Adds an availability test for the functionality polyfilled. * Adds documentation in the `README` about the differences in the functionality of the `expectUserDeprecation*()` methods in PHPUnit <= 9 versus PHPUnit 11. Includes: * Adding the new polyfill to the existing `TestCases` classes. This commit also drops support for PHPUnit 10: * Make the version drop explicit via the PHPUnit `require` settings in the `composer.json` file. * Update the GH Actions `test` workflow to no longer run the tests against PHPUnit 10. This also allows for getting rid of a number of work-arounds which were previously needed for running the tests on both PHPUnit 10.0 and PHPUnit 10.1+. * Updates the PHPUnit 10+ configuration file used by the polyfills package itself to the schema supported in PHPUnit 11. This includes adding some configuration options to the file, which were previously, conditionally, added in the CI run as those options were not available in PHPUnit 10.0. Refs: * sebastianbergmann/phpunit 5605
jrfnl
force-pushed
the
feature/3.x/new-expectuserdeprecation-polyfill-trait
branch
from
September 6, 2024 21:16
01cfa57
to
37f2e99
Compare
hellofromtonya
deleted the
feature/3.x/new-expectuserdeprecation-polyfill-trait
branch
September 6, 2024 21:19
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PHPUnit 11.0.0 introduces the new
TestCase::expectUserDeprecationMessage()
andTestCase::expectUserDeprecationMessageMatches()
methods.These methods can largely be seen as replacements for the
TestCase::expectDeprecationMessage()
andTestCase::expectDeprecationMessageMatches()
methods which were removed in PHPUnit 10.0, though there are significant differences between the implementation details of the old vs the new methods.As PHPUnit 10 does not have any even semi-equivalent method(s) available, the challenge was to polyfill these methods for PHPUnit 10. And as these methods are expectation methods, not assertions, this is even more challenging as the information to evaluate a pass/fail is not available at the time of the method call, but only once the rest of the test code has run.
To do so, the following five options were considered:
Option 1: evaluate pass/fail in an "assertPostConditions()" method
While possible, doing so would make the use of the
Yoast\PHPUnitPolyfills\TestCases\TestCase
base class required. It would also mean deprecating/removing the annotation-basedYoast\PHPUnitPolyfills\TestCases\XTestCase
class as there is no annotation available to mark a method as an "assertPostConditions()" method, so the functionality could not work with theXTestCase
class.As a knock-on effect of making the use of the Polyfilled
TestCase
a requirement, it would also mean that using of the individual Polyfills as stand-alone traits should be deprecated/no longer be supported.This chain of consequences was deemed undesirable as it breaks the premise of the Polyfills being a "drop in" helper library.
Option 2: use Events to evaluate pass/fail
While possible, doing so would mean that the event listeners would need to be "hooked in" from the PHPUnit config file. This would mean that every project using the Polyfills and wanting to use the
expectUserDeprecation*()
methods would need to make changes to their PHPUnit configuration file. Additionally, adding the event listeners would make a configuration file incompatible with PHPUnit <= 9 and the event listener should not be hooked in for PHPUnit 11, in which the functionality exists in PHPUnit natively. In other words, it would mean that end-users would now need three (3) different PHPUnit configuration files, one for PHPUnit <=9, one for PHPUnit 10 and one for PHPUnit 11.Again, this was deemed undesirable as it breaks the premise of the Polyfills being a "drop in" helper library.
Option 3: polyfill, but don't support PHPUnit 10 in the polyfill
This would basically mean that any test using the
expectUserDeprecation*()
polyfill would be marked as skipped on PHPUnit 10.This is not a transparent action and the impact of this on tests was deemed too high to be acceptable.
Option 4: don't polyfill
It should be obvious that this is the least desirable option as this would mean the polyfills wouldn't polyfill (at least for these methods).
Option 5: polyfill, but drop support for PHPUnit 10 completely
This means polyfilling for PHPUnit <= 9 by dropping through to the "old" functions and not supporting installation of the Polyfills 3.x versions in combination with PHPUnit 10.
In practical terms, the net effect of this is that tests on PHP 8.1 would run on PHPUnit 9 instead of PHPUnit 10. Other than that, there is no downside.
Having considered these options carefully, option 5 was deemed most appropriate and most in line with the simple elegance of the functionality offered by the PHPUnit Polyfills until now.
Final note: when falling through to the "old" methods on PHPUnit 9.5.x, PHPUnit native deprecation notices will be shown (but won't fail a build). Unfortunately, this is unavoidable as the
@
operator is not respected and cannot silence these deprecation notices. Having said that, as long as the polyfilled methods are used in the actual test suite, these deprecation notices can be safely ignored anyway.This commit implements option 5:
use
-ing the trait in PHPUnit versions in which the methods are already natively available.README
about the differences in the functionality of theexpectUserDeprecation*()
methods in PHPUnit <= 9 versus PHPUnit 11.Includes:
TestCases
classes.This commit also drops support for PHPUnit 10:
require
settings in thecomposer.json
file.test
workflow to no longer run the tests against PHPUnit 10. This also allows for getting rid of a number of work-arounds which were previously needed for running the tests on both PHPUnit 10.0 and PHPUnit 10.1+.Refs:
expectUserDeprecationMessage()
andexpectUserDeprecationMessageMatches()
to expectE_USER_DEPRECATED
issues sebastianbergmann/phpunit#5605