-
Notifications
You must be signed in to change notification settings - Fork 15
Composer/GH Actions: allow for PHPUnit 12.x and make the tests cross-version compatible #247
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
Merged
Conversation
This file contains hidden or 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
…t 12)
Background
PHPUnit 11.2 deprecates the use of the `#[CoversClass]` attribute for targetting traits, in favour of a new `#[CoversTrait]` attribute, and as of PHPUnit 12, using `#[CoversClass]` for traits is no longer supported.
Now even within the PHPUnit 11.x series, this led to some confusion with PHPUnit 11.2.0 introducing the `#[CoversTrait]` attribute and the deprecation, PHPUnit 11.4.0 deprecating the `#[CoversTrait]` attribute again and PHPUnit 11.5.4 re-introducing the original deprecation.
Additionally, as PHPUnit originally incorrectly didn't ignore unknown attributes, using the `#[CoversTrait]` attribute when tests would need to run on PHPUnit < 11.2.0 (i.e. tests running on PHPUnit 10 + 11), would lead to a fatal "class not found" error.
This was fixed in PHPUnit 11.3.6 and 10.5.35, but affects earlier PHPUnit 10/11 versions.
The problem
This is a problem for the PHPUnit Polyfills package as it:
* Primarily uses traits for its functionality;
* AND uses strict code coverage, i.e. _requires_ coverage metadata;
* AND **needs** to run the tests on older PHPUnit minors to safeguard its functionality properly.
Case in point: the `assertObjectNotEquals()` assertion was introduced in PHPUnit 11.2.0, so the polyfill for this assertion needs to work on PHPUnit 7, 8, 9 and 11.0 - 11.1. To safeguard the polyfill properly, the tests for it **must** also be run on PHPUnit 11.0 < 11.2.0.
However, if the `#[CoversTrait]` attribute would be used with PHPUnit 11.0 < 11.2.0, it would cause the above mentioned fatal "class not found" error, but without the `#[CoversTrait]` attribute, the tests would throw a PHPUnit warning on PHPUnit 12 and code coverage would not be recorded correctly.
So now we have a conundrum: how to get the tests running and passing on all supported PHPUnit versions ?
> Note: while this is a blocking problem for the test suite of the PHPUnit Polyfills package, it is not expected that many userland libraries will run into this as those will a) normally only use the latest point releases of each major and b) don't use traits that often.
> Having said that, userland tests covering traits, which need to run on PHPUnit 10 + 11 + 12 will run into the same problem.
The solution
There are various solutions I considered and tried, which didn't work, like:
* Having both attributes on the tests - this still runs into the fatal error on PHPUnit < 11.2.0 -.
* Using `#[CoversTrait]` exclusively and, still running the tests on PHPUnit < 11.2.0, but not running code coverage on PHPUnit < 11.2.0 - again, this still runs into the fatal error.
For now, the solution I've come up with is multi-pronged:
1. Introduce PHPUnit version specific test wrappers.
This basically comes down to:
1. Changing the original test classes to `abstract` `TestCase`s.
2. Having PHPUnit version specific wrapper `Test` classes for each of these `TestCase`s, where the only function of the "wrapper" class is to add the applicable PHPUnit annotations and attributes.
For our current purposes, we need two "wrapper" classes for each test:
- One for PHPUnit <= 11, which has the docblock annotations for PHPUnit 7-9 and the `#[CoversClass]` attributes for PHPUnit 11.
- One for PHPUnit 12, which has the `#[CoversTrait]` attributes.
2. Introduce yet another PHPUnit config file to manage which tests are being run on which PHPUnit version.
Having yet another config file and multiple test classes for each polyfill isn't great from a maintainability perspective, but I can't come up with a simpler solution which would still allow the tests to run properly PHPUnit cross-version, other than to stop requiring `@covers` annotations/attributes (which is undesirable).
Notes about the test wrapper classes:
* While working on this, I found that class level attributes MUST be on the concrete test class. Class level PHPUnit attributes on an abstract `TestCase` are not respected.
This means that some duplication of attributes is needed in the wrapper classes , as we cannot leave the "common"/shared attributes on the `TestCase`.
* Even though this is handled primarily via the split PHPUnit config files, all wrapper classes still have a `@requires PHPUnit` annotation + attribute in an attempt to make error messaging more informative if the test suites would be called incorrectly, i.e. if a config file is used with the wrong PHPUnit version.
* There are a number of resource related test classes which require PHP < 7.4/8.0/8.1, so will never run on PHPUnit 12, which has a minimum PHP version of PHP 8.3. These tests have been omitted from the `WrappersGte12` folder.
Notes about splitting the test config:
As of PHPUnit 11.0, a test can no longer be part of multiple test suites.
This means that we would not be able to define one test suite for each PHPUnit version as the cross-version compatible tests would have to be repeated in each test suite, which is not allowed.
Now, we _could_ have a setup, close to the current one, with a `Crossversion` test suite and separate test suites for each PHPUnit version, but that would mean that each test run would now always need an extra `--testsuite` CLI argument to ensure the right tests are run per PHPUnit version.
This would mean extra cognitive load for contributors and the need for additional logic in the `test` workflow in CI.
> Note: the `testsuite` `directory` node in the XML files do allow for setting a `phpVersion`, but do not allow for setting a `phpUnitVersion`, so we cannot limit certain directories to certain PHPUnit versions via configuration alone.
As the pre-existing setup already uses multiple configuration files, it felt more intuitive and more contributor-friendly to expand on that setup and introduce a third config file instead. This also eliminates the need for additional logic in the CI workflow, as the pre-existing logic for determining which test config file to use can be re-used.
Also note that the `TestListeners` tests are only included in the PHPUnit <= 9 config, as, at this time, the test listener setup is (still) not compatible with PHPUnit > 9.
Refs:
* https://github.com/sebastianbergmann/phpunit/blob/11.2.9/ChangeLog-11.2.md#1120---2024-06-07
* sebastianbergmann/phpunit 5799
* https://github.com/sebastianbergmann/phpunit/blob/11.4.4/ChangeLog-11.4.md#1140---2024-10-05
* https://github.com/sebastianbergmann/phpunit/blob/11.5/ChangeLog-11.5.md#1154---2025-01-28
* sebastianbergmann/phpunit 5958
* https://github.com/sebastianbergmann/phpunit/blob/10.5/ChangeLog-10.5.md#10535---2024-09-19
* https://github.com/sebastianbergmann/phpunit/blob/11.3.6/ChangeLog-11.3.md#1136---2024-09-19
* sebastianbergmann/phpunit 5937
* https://github.com/sebastianbergmann/phpunit/blob/11.0.10/ChangeLog-11.0.md#1100---2024-02-02
* sebastianbergmann/phpunit@57e07ee
* https://docs.phpunit.de/en/11.5/configuration.html#the-testsuites-element
PHPUnit 12.0 has been released.
As the PHPUnit Polyfills, as of now, will officially support PHPUnit 12.x (with the exception of the TestListeners), the GH Actions workflow should be updated to reflect this.
This commit:
* Allows for PHPUnit 12 in the `composer.json` `require`.
* Add builds for PHP 8.3 and 8.4 against low PHPUnit 12 versions for the Composer based tests.
The "high" versions are automatically sorted via the matrix `auto` setting in combination with the Composer PHPUnit version requirements being widened.
* Add builds for PHP 8.3 and 8.4 against high/low PHPUnit 12 for the PHAR based tests.
* Add an extra experimental build in both test workflows against PHP "nightly" to ensure both PHPUnit 9.x, 11.x, as well as PHPUnit 12.x are tested with PHP 8.5.
Refs:
* https://phpunit.de/announcements/phpunit-12.html
* https://github.com/sebastianbergmann/phpunit/blob/12.0/ChangeLog-12.0.md
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.
Tests: allow for the tests to run cross-version (PHPUnit <12 + PHPUnit 12)
Background
PHPUnit 11.2 deprecates the use of the
#[CoversClass]attribute for targetting traits, in favour of a new#[CoversTrait]attribute, and as of PHPUnit 12, using#[CoversClass]for traits is no longer supported.Now even within the PHPUnit 11.x series, this led to some confusion with PHPUnit 11.2.0 introducing the
#[CoversTrait]attribute and the deprecation, PHPUnit 11.4.0 deprecating the#[CoversTrait]attribute again and PHPUnit 11.5.4 re-introducing the original deprecation.Additionally, as PHPUnit originally incorrectly didn't ignore unknown attributes, using the
#[CoversTrait]attribute when tests would need to run on PHPUnit < 11.2.0 (i.e. tests running on PHPUnit 10 + 11), would lead to a fatal "class not found" error.This was fixed in PHPUnit 11.3.6 and 10.5.35, but affects earlier PHPUnit 10/11 versions.
The problem
This is a problem for the PHPUnit Polyfills package as it:
Case in point: the
assertObjectNotEquals()assertion was introduced in PHPUnit 11.2.0, so the polyfill for this assertion needs to work on PHPUnit 7, 8, 9 and 11.0 - 11.1. To safeguard the polyfill properly, the tests for it must also be run on PHPUnit 11.0 < 11.2.0.However, if the
#[CoversTrait]attribute would be used with PHPUnit 11.0 < 11.2.0, it would cause the above mentioned fatal "class not found" error, but without the#[CoversTrait]attribute, the tests would throw a PHPUnit warning on PHPUnit 12 and code coverage would not be recorded correctly.So now we have a conundrum: how to get the tests running and passing on all supported PHPUnit versions ?
The solution
There are various solutions I considered and tried, which didn't work, like:
#[CoversTrait]exclusively and, still running the tests on PHPUnit < 11.2.0, but not running code coverage on PHPUnit < 11.2.0 - again, this still runs into the fatal error.For now, the solution I've come up with is multi-pronged:
This basically comes down to:
abstractTestCases.Testclasses for each of theseTestCases, where the only function of the "wrapper" class is to add the applicable PHPUnit annotations and attributes.For our current purposes, we need two "wrapper" classes for each test:
#[CoversClass]attributes for PHPUnit 11.#[CoversTrait]attributes.Having yet another config file and multiple test classes for each polyfill isn't great from a maintainability perspective, but I can't come up with a simpler solution which would still allow the tests to run properly PHPUnit cross-version, other than to stop requiring
@coversannotations/attributes (which is undesirable).Notes about the test wrapper classes:
TestCaseare not respected.This means that some duplication of attributes is needed in the wrapper classes , as we cannot leave the "common"/shared attributes on the
TestCase.@requires PHPUnitannotation + attribute in an attempt to make error messaging more informative if the test suites would be called incorrectly, i.e. if a config file is used with the wrong PHPUnit version.WrappersGte12folder.Notes about splitting the test config:
As of PHPUnit 11.0, a test can no longer be part of multiple test suites.
This means that we would not be able to define one test suite for each PHPUnit version as the cross-version compatible tests would have to be repeated in each test suite, which is not allowed.
Now, we could have a setup, close to the current one, with a
Crossversiontest suite and separate test suites for each PHPUnit version, but that would mean that each test run would now always need an extra--testsuiteCLI argument to ensure the right tests are run per PHPUnit version.This would mean extra cognitive load for contributors and the need for additional logic in the
testworkflow in CI.As the pre-existing setup already uses multiple configuration files, it felt more intuitive and more contributor-friendly to expand on that setup and introduce a third config file instead. This also eliminates the need for additional logic in the CI workflow, as the pre-existing logic for determining which test config file to use can be re-used.
Also note that the
TestListenerstests are only included in the PHPUnit <= 9 config, as, at this time, the test listener setup is (still) not compatible with PHPUnit > 9.Refs:
Composer/GH Actions: allow for PHPUnit 12.x
PHPUnit 12.0 has been released.
As the PHPUnit Polyfills, as of now, will officially support PHPUnit 12.x (with the exception of the TestListeners), the GH Actions workflow should be updated to reflect this.
This commit:
composer.jsonrequire.The "high" versions are automatically sorted via the matrix
autosetting in combination with the Composer PHPUnit version requirements being widened.Refs: