Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Optionally ignore PHPUnit deprecations when determining the test runner's shell exit code #5937

Closed
jrfnl opened this issue Sep 3, 2024 · 2 comments
Closed

Optionally ignore PHPUnit deprecations when determining the test runner's shell exit code #5937

jrfnl opened this issue Sep 3, 2024 · 2 comments
Assignees
@sebastianbergmann
Labels
feature/configuration/cli feature/configuration/xml feature/test-runner CLI test runner type/enhancement A new idea that should be implemented version/10 Something affects PHPUnit 10 version/11 Something affects PHPUnit 11

Comments

@jrfnl
Copy link
Contributor

jrfnl commented Sep 3, 2024 •
edited
Loading

Q A
PHPUnit version 10 + 11
PHP version any supported
Installation Method either

Summary

Use case

Given a test suite which:

  • Needs to run on PHPUnit 10 + 11
  • AND the source code includes traits which are covered by tests
  • AND the test suite needs to fail the build when PHP native deprecation notices are seen...

How can I make the tests pass and code coverage be recorded on both PHPUnit 10 + 11 ?

The underlying problem

This is actually part of a larger problem which has existed since PHPUnit 10.0: deprecations coming from PHPUnit itself affecting the exit code.

Now, I know this was a deliberate choice (based on comments by @sebastianbergmann in various tickets over time) and as there have been work-arounds until now, I have not spoken up about this before, but with the deprecation of #[CoversClass] in favour of #[CoversTrait] this is now an insurmountable problem.

The typical use-case for the failOn* settings is (open source) libraries which need to run on multiple PHP versions and want to prevent (new) PHP deprecations/notices/warnings from creeping into their code.

Up to now, the fact that PHPUnit deprecations affected the exit code meant that those libraries would need:

  • To do upgrades to their test suite on every PHPUnit version which introduced a new deprecation.
    In contrast to the previous (PHPUnit <= 9) behaviour where those deprecations could be ignored until the next major PHPUnit release.
  • (in most cases) Have an extra --migration-configuration step in the CI (with continue-on-error) just to be sure that their test run would not fail on an outdated (cross-version compatible) configuration.
  • If still supporting PHPUnit 9, those projects would also need two separate PHPUnit config file as the migration doesn't handle the convertDeprecationsToExceptions conversion to displayDetailsOnTestsThatTriggerDeprecations + failOnDeprecation.
  • etc

To me, the fact that PHPUnit deprecations affect the exit code is in stark contrast to the changes made in PHPUnit 9.5.x to no longer fail tests on PHP deprecation notices by default and in PHPUnit 10 to do the same for PHP warnings and notices.
The argument for that change was that most packages should be able to ignore those deprecations (notices/warnings) until the next PHP major, so these notices should not be a reason to fail the tests.

By that same logic, I should be able to ignore PHPUnit deprecation notices until the next PHPUnit major, but I no longer can if I do want to see PHP deprecation notices.

Current behavior

As things are, the test run will always fail on PHPUnit 11.x due to the PHPUnit deprecation notice "Targeting a trait such as * with #[CoversClass] is deprecated, please refactor your test to use #[CoversTrait] instead." and exit with exit code 1.

How to reproduce

I've created a reproduction sample here: https://github.com/jrfnl/bug-report-reproduction-scenarios/commits/phpunit/5937-fail-on-deprecation/
CI run on the original test code: https://github.com/jrfnl/bug-report-reproduction-scenarios/actions/runs/10683893211

Things I've considered/tried to see if I could find a work-around (also available as commits in the reproduction sample):

Maybe I'm overlooking yet another option and there is work-around, in which case: great! but I haven't been able to find one.

Expected behavior

The most optimal solution would be one where PHPUnit deprecation notices do not affect the exit code of the test run ever.

The second most optimal solution would be some sort of configuration setting in the XML file and via a CLI flag to ignore PHPUnit deprecation notices.
Yes, this would also need a CLI flag as if this is only available via the XML config, the XML file would cause failing builds on older PHPUnit versions due to invalid configuration.

A much less optimal, but sort-of-workable solution would be one where the deprecation notice for using #[CoversClass] on traits is not thrown when there is also a #[CoversTrait] attribute on the same test class, in combination with ignoring attributes which are not available in older PHPUnit versions.

Additional context

Closely related issues:
@jrfnl jrfnl added the type/bug Something is broken label Sep 3, 2024
jrfnl added a commit to Yoast/PHPUnit-Polyfills that referenced this issue Sep 3, 2024
@jrfnl
CI: update for PHPUnit 11.x support
5ac3d53 
As the PHPUnit Polyfills, as of now, will officially support PHPUnit 11.x, with the exception of the TestListeners, the GH Actions workflow should be updated to reflect this.

This commit:
* Move the PHP 8.2/8.3 "high" (auto) PHPUnit version builds out of the matrix and run these without code coverage (see below).
* Add builds for PHP 8.2 and 8.3 against low PHPUnit 11 versions for the Composer based tests.
* Add builds for PHP 8.2 and 8.3 against high/low PHPUnit 11 for the PHAR based tests.
* Add an extra experimental build in both test workflows against PHP "nightly" to ensure both PHPUnit 9.x, 10.x, as well as PHPUnit 11.x are tested with PHP 8.4.
* Updates the experimental build against "future" PHPUnit to always run against the latest official PHP release.

**Regarding PHPUnit 11 and running code coverage**:

Since PHPUnit 10, PHPUnit does not distinguish between PHPUnit and PHP deprecation notices anymore.
This means that when `failOnDeprecation` is enabled (as is done for this library to be ready early for new PHP versions), a test run will also fail if there are PHPUnit native deprecation notices.

Now PHPUnit 11.2 deprecated the use of `#[CoversClass]` for traits and introduced a `#[CoversTrait]` attribute to replace this.
However, it is currently impossible to action this deprecation notice in a PHPUnit cross-version compatible manner.

This has been reported upstream and until that issue has been addressed in PHPUnit itself, the net-effect of this issue is that we can run the tests with code coverage on PHPUnit < 11.2, but not on PHPUnit 11.2 or higher.

Ref:
* sebastianbergmann/phpunit#5937
@sebastianbergmann sebastianbergmann changed the title PHPUnit deprecations affecting the exit code means failOnDeprecation setting can not be used (concrete use case inside) Optionally ignore PHPUnit deprecations when determining the test runner's shell exit code Sep 4, 2024
@sebastianbergmann sebastianbergmann self-assigned this Sep 4, 2024
@sebastianbergmann sebastianbergmann added type/enhancement A new idea that should be implemented feature/test-runner CLI test runner version/10 Something affects PHPUnit 10 feature/configuration/xml feature/configuration/cli version/11 Something affects PHPUnit 11 and removed type/bug Something is broken labels Sep 4, 2024
@sebastianbergmann
Copy link
Owner

Now, I know this was a deliberate choice (based on comments by @sebastianbergmann in various tickets over time) and as there have been work-arounds until now, I have not spoken up about this before, but with the deprecation of #[CoversClass] in favour of #[CoversTrait] this is now an insurmountable problem.

Thank you for taking the time to research this issue and for explaining it so thoroughly. I only wish you had brought this up sooner, as my deliberate choice you mention was obviously (just not to me) wrong.

The next versions of PHPUnit 10.5 and PHPUnit 11.3 will have a separation between PHPUnit deprecations and E_DEPRECATED / E_USER_DEPRECATED when it comes to deciding the test runner's exit code and the following configurations are possible (via CLI and XML):

  • do not fail on E_DEPRECATED / E_USER_DEPRECATED and do not fail on PHPUnit deprecation
  • fail on E_DEPRECATED / E_USER_DEPRECATED, but do not fail on PHPUnit deprecation
  • do not fail on E_DEPRECATED / E_USER_DEPRECATED, but fail on PHPUnit deprecation
  • fail on E_DEPRECATED / E_USER_DEPRECATED and fail on PHPUnit deprecation

@jrfnl
Copy link
Contributor Author

jrfnl commented Sep 4, 2024

Thank you for taking the time to research this issue and for explaining it so thoroughly. I only wish you had brought this up sooner, as my deliberate choice you mention was obviously (just not to me) wrong.

@sebastianbergmann Sorry for that, but seeing that other people's issues about this were being dismissed, I didn't think opening yet another issue about this would be useful until there was a situation which could potentially convince you 😕

The next versions of PHPUnit 10.5 and PHPUnit 11.3 will have a separation between PHPUnit deprecations and E_DEPRECATED / E_USER_DEPRECATED when it comes to deciding the test runner's exit code and the following configurations are possible (via CLI and XML):

💞 🎉 Thank you!

@sebastianbergmann sebastianbergmann mentioned this issue Sep 27, 2024
Closed
@jrfnl jrfnl mentioned this issue Oct 14, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sebastianbergmann sebastianbergmann

Labels
feature/configuration/cli feature/configuration/xml feature/test-runner CLI test runner type/enhancement A new idea that should be implemented version/10 Something affects PHPUnit 10 version/11 Something affects PHPUnit 11
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@sebastianbergmann @jrfnl