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.

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

Handling of configuration files #134

Open
anomiex opened this issue Jun 9, 2023 · 4 comments
Open

Handling of configuration files #134

anomiex opened this issue Jun 9, 2023 · 4 comments

Comments

@anomiex
Copy link

anomiex commented Jun 9, 2023

PHPUnit 10 brings both several changes to the configuration file and a misfeature of causing phpunit to exit with a non-zero status if the config file does not strictly validate.

So, for example, if you had been relying on convert{X}ToExceptions="true", you might want to set displayDetailsOnTestsThatTrigger{X}="true" and failOn{X}="true" for PHPUnit 10 to maintain that behavior going forward. But unless you also remove convert{X}ToExceptions="true", changing the behavior for runs with earlier versions of PHP, PHPUnit 10 will whine about not recognizing the old setting and will exit with a failure status even if all actual tests pass.

I don't know what the best way around this might be.

Simplest might be to provide a wrapper script that, if not passed -c or --configuration or --no-configuration, looks for various config file names and picks the best one for the current version of PHPUnit to set --configuration before execing phpunit. For example, it might check for any "phpunit.{VER}.xml.dist" and pick the one with the highest {VER} that's not greater than PHPUnit's own version.

A little fancier might be a wrapper that determines the configuration file in the same way PHPUnit would, copies it to a temporary file while stripping a predefined list of settings based on the PHPUnit version, and calls phpunit with --configuration pointing to the temporary file.

Unfortunately PHPUnit doesn't provide any way to hook into its own code to handle things more directly. There might still be ways, but they'd be fragile.

@jrfnl
Copy link
Collaborator

jrfnl commented Jun 9, 2023

@anomiex PHPUnit offers a --migrate-configuration option since PHPUnit 9.3, which can be run before running PHPUnit for the test run, to convert a configuration file to a more recent format.
While definitely not perfect, as it doesn't handle the convert*ToExceptions settings, it is the recommended way to work with PHPUnit 9.x vs 10.x configuration files.

I think improving that --migrate-configuration option in PHPUnit should be explored first, before consideration should be given to adding any such functionality to the PHPUnit Polyfills.

I do feel your pain in this though, but there are more sticky issues around this as the PHPUnit 10.x failOn* options also do not translate to the same run results (exit code) in PHPUnit 9.x with convert*ToExceptions as in PHPUnit 9.x only PHP native errors/notices etc were taken into account for the exit code, while in PHPUnit 10.x, PHPUnit native ones are also taken into account, making any setup using the failOn* options brittle.

I think this needs further discussion in the PHPUnit repo first. If you do open an issue there, please add the link to this ticket as I'd very much would like to follow the discussion.

In the mean time, here are some links for interim solutions I've used in certain projects (take note of the info in the commit messages):

@anomiex
Copy link
Author

anomiex commented Jun 9, 2023

Thanks for pointing out that failOnDeprecation will also fail on PHPUnit's own deprecation messages, I had missed that.

Someone already filed sebastianbergmann/phpunit#5199 about thee test runner exiting with a non-zero code on warnings even when failOnWarning is false, so it seems unlikely he'd do anything about not wanting it to exit with a non-zero code when failOnWarning is true.

Also I'm having a hard time coming up with a reason for wanting to ignore PHPUnit's own deprecation warnings but not PHP deprecation warnings that's more compelling than "we'd have to wait on upgrading phpunit until yoast/phpunit-polyfills gets a polyfill", except for the "deprecation" about an older-schema config file.

I filed sebastianbergmann/phpunit#5405 for one actual bug I've encountered in the migration.

I'm skeptical he'll consider failing to migrate convert*ToExceptions to failOn* a bug, but I filed sebastianbergmann/phpunit#5406 anyway.

@jrfnl
Copy link
Collaborator

jrfnl commented Jun 9, 2023

@anomiex Thanks for that. I've subscribed to both issues. I hadn't gotten around to do a proper write-up of the issues myself yet.

I'm having a hard time coming up with a reason for wanting to ignore PHPUnit's own deprecation warnings but not PHP deprecation warnings that's more compelling than "we'd have to wait on upgrading phpunit until yoast/phpunit-polyfills gets a polyfill"

Except that is not the real issue.

There have been multiple times in the past when PHPUnit would warn about something being deprecated without replacement, only for it to be reconsidered and a replacement being added after all at a later point in time.

This happened even as recently as PHPUnit 9.6 which warns about assertObjectHasAttribute() being deprecated without replacement. That deprecation got adjusted in PHPUnit 9.6.7 after the release of PHPUnit 10.1 which introduced the assertObjectHasProperty() method.

There are also warnings ("data providers have to be static") which don't necessarily affect the running of the test suite until a later point in time.

Along the same lines as the reasoning for PHP deprecations, you may want to ignore those until those can be addressed as a managed mini-project, but that doesn't necessarily mean that PHP and PHPUnit deprecations should be addressed in the same mini-project or at the same time.

They have a different time line and a different scope.

Additionally, PHPUnit has recently had a tendency to change the configuration file a lot and to add deprecations in minor/patch versions which come out fairly regularly, which is very different from PHP, where such deprecations are only added once a year in a minor.

What this means in practice is that, while for a new PHP version, you may add a CI build once a year with a continue-on-error for the time being until all deprecations have been fixed (managed), a new PHPUnit minor/patch version may now start failing builds at random (uncontrollable unless you fix the max PHPUnit version in Composer and let Dependabot manage the updates every week or so, which is a complete drag).

In other words, the current PHPUnit 10.x XML attributes to manage warnings/deprecations etc just do not allow for updating PHPUnit related issues in a controlled way without blocking automatic use of the latest PHPUnit version.

Does that clarify things a little more ?

@anomiex
Copy link
Author

anomiex commented Jun 10, 2023

which is very different from PHP, where such deprecations are only added once a year in a minor.

That's a good point, PHP does limit their deprecations to a new minor. And we (like probably everyone else) tend to treat minor PHP versions similarly to major versions of other stuff like PHPUnit.

Sounds like you should handle filing the PHPUnit bug about this one, you have a much better grasp on the issue. 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants