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

PHPUnit tests with @runInSeparateProcess fail with PHP 8.0 and Windows #8

Open
1 of 4 tasks
pondermatic opened this issue Aug 11, 2021 · 5 comments
Open
1 of 4 tasks
Assignees

Comments

@pondermatic
Copy link
Contributor

Reproduction Steps

The LifterLMS unit tests that use the @runInSeparateProcess annotation return an error and are not tested when run with PHP 8.0 and Windows 10.

The tests are successful if run with PHP 7.2 or the @runInSeparateProcess annotation is removed.

Expected Behavior

The tests pass.

Actual Behavior

The tests fail with a "The filename, directory name, or volume label syntax is incorrect." message.

Error Messages / Logs

"The filename, directory name, or volume label syntax is incorrect."

This issue has been recreated:

  • Locally
  • On a staging site
  • On a production website
  • With only LifterLMS and a default theme
@thomasplevy
Copy link
Contributor

@pondermatic have you found anything actionalable here... @runInSeparateProcess is a documented and acceptable-to-use annotation in PHPHUNIT: https://phpunit.readthedocs.io/en/9.5/annotations.html?highlight=%40runInSeparateProcess#runinseparateprocess

I can't find anything online about a windows 10 conflict there

Removing the annotation will cause issues because when using this annotation we generally test things that will affect the PHP process and cause other tests to behave in unexpected ways, usually this is done when testing things related to constants or singleton instances

@pondermatic
Copy link
Contributor Author

It appears that the current solution is to run LifterLMS unit tests with PHP 7.3 as that is the last version that is compatible with PHPUnit 7.5.

Until WordPress and LifterLMS transition to a newer version of PHPUnit, it may be possible to use a modified version of PHPUnit 7.5 as @konamiman at Automattic has done, although that version does not fix this issue and it adds additional complexity to this project for probably very little gain.

@thomasplevy
Copy link
Contributor

@pondermatic if that's the case then this trac ticket (on my immediate priority list) is going to help us resolve this: https://core.trac.wordpress.org/ticket/46149

The WP Core is making big steps towards reducing the enumerable issues inherent in trying to run PHP Unit with WordPress across the various WP versions that need supporting. I started working on switching us to use this library (which the WP Core is now using on the nightly branch) https://github.com/Yoast/PHPUnit-Polyfills

I got stuck and a recent commit (two days ago) will make it possible for us to switch over to using this which I hope will resolve this too.

If you can manage to deal with this for a bit I'll hopefully have us switched over in a week or two... I'll work on it today a bit and have a better idea on timelines.

@thomasplevy thomasplevy self-assigned this Aug 11, 2021
@pondermatic
Copy link
Contributor Author

I can easily switch unit testing to use PHP 7.2 instead of PHP 8.0.

There is one test with a trailing comma in the arguments of a function call, which breaks if PHP < 8.0. Otherwise all tests run. 3 are skipped and 1 is risky, but 0 failures.

@thomasplevy
Copy link
Contributor

@pondermatic check this branch out, if you have a moment: https://github.com/gocodebox/lifterlms/tree/phpunit-polyfill

  • If installing on php 7.x, run composer install --ignore-platofmr-reqs, on php 8.x composer install suffices.
  • Install the tests with WP_TESTS_VERSION=trunk composer run tests-install -- this will force the tests lib to be pulled in from the WP Core nightly (which isn't backported yet)
  • Run tests composer run tests

If the problem we're encountering on Windows is PHPUnit version compatibility with Windows we should be good to go.

Also, for whatever it's worth, there already are existing solutions to run PHPUnit on PHP 8 despite version support, we run Travis Tests against PHP 8 this way. It's convoluted and complicated and it's going to stop working with PHP 8.1 which is why I've been following the core trac ticket and keeping my eye on getting this situated in a less insane way...

Once the WP core changes are backported (they're talking about backporting this to 5.2) I'll restructure the composer scripts, travis tests, etc... and we'll switch to using this across the board (other add-ons, etc...)

@thomasplevy thomasplevy transferred this issue from gocodebox/lifterlms Aug 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants