Skip to content

Conversation

@jrfnl
Copy link
Collaborator

@jrfnl jrfnl commented Feb 9, 2025

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:

  • 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 TestCases.
    2. Having PHPUnit version specific wrapper Test classes for each of these TestCases, 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:

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:

  • 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:

jrfnl added 2 commits February 9, 2025 16:07
…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
@jrfnl jrfnl added this to the 4.0.0 milestone Feb 9, 2025
@jrfnl jrfnl mentioned this pull request Feb 9, 2025
9 tasks
@coveralls
Copy link

Coverage Status

coverage: 98.331%. remained the same
when pulling acc6556 on feature/composer-ci-allow-for-phpunit-12
into a4f58ea on 4.x.

@jrfnl jrfnl merged commit 85687d5 into 4.x Feb 9, 2025
151 checks passed
@jrfnl jrfnl deleted the feature/composer-ci-allow-for-phpunit-12 branch February 9, 2025 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants