gocodebox / lifterlms-tests

LifterLMS Tests is a project to help bootstrap automated testsing in LifterLMS projects.
2 stars 2 forks source link

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

Open pondermatic opened 3 years ago

pondermatic commented 3 years ago

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:

thomasplevy commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

@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.

pondermatic commented 3 years ago

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 commented 3 years ago

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

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...)