Add support to read WP_TESTS_PHPUNIT_POLYFILLS_PATH from environment variables

[shivammathur]

I would like to add support to read the WP_TESTS_PHPUNIT_POLYFILLS_PATH from an environment variable.

Currently one has to define the WP_TESTS_PHPUNIT_POLYFILLS_PATH as a PHP constant. In a CI setup like GitHub Actions, this can be done only once the WordPress test suite has been installed.

Adding support for reading this from an environment variable would allow setup-php GitHub Action to set this variable as it is mostly run before the WordPress test suite is installed.

This PR adds support to read the WP_TESTS_PHPUNIT_POLYFILLS_PATH from an environment variable.

Currently one has to define the WP_TESTS_PHPUNIT_POLYFILLS_PATH as a PHP constant.

Adding support for reading this from an environment variable would allow setup-php GitHub Action to set this variable.

Trac ticket: https://core.trac.wordpress.org/ticket/54470

[audrasjb]

Hello, welcome to WordPress Trac and thank you for opening this ticket,

I just wanted to note that there is a few coding standards issues in the proposed pull request:
`
Error: Expected 1 space(s) after closing parenthesis; found " ) "
Error: Expected 1 spaces after opening parenthesis; 0 found
Error: String "WP_TESTS_PHPUNIT_POLYFILLS_PATH" does not require double quotes; use single quotes instead
Error: Expected 1 spaces before closing parenthesis; 0 found
`

For more details, see:

@jrfnl

Please review this PR, once merged I can set WP_TESTS_PHPUNIT_POLYFILLS_PATH when phpunit-polyfills is installed in setup-php, Then it will be easier for users to use their own PHPUnit in tools input of setup-php.

[shivammathur]

Replying to audrasjb:

Hello, welcome to WordPress Trac and thank you for opening this ticket,

I just wanted to note that there is a few coding standards issues in the proposed pull request:

Error: Expected 1 space(s) after closing parenthesis; found " ) "
Error: Expected 1 spaces after opening parenthesis; 0 found
Error: String "WP_TESTS_PHPUNIT_POLYFILLS_PATH" does not require double quotes; use single quotes instead
Error: Expected 1 spaces before closing parenthesis; 0 found

Fixed. Thanks

[jrf]

👋🏻 Hi @shivammathur!

I'm not opposed to this proposal, but I do not fully understand the use-case. Could you elaborate a little on that ?

The way things were designed to work and have been set up, the constant is only supposed to be used by plugins/themes running integration tests with the WP Core test framework. WP Core itself does not need it.

As plugins and themes in that situation will always need a custom test bootstrap file anyway - to load the WP Core boostrap file + to register the plugin/theme with WP -, declaring the constant from within their test bootstrap file ahead of loading the WP Core bootstrap - or even in their ​`phpunit.xml.dist` file - should not be a problem.

Aside from that, in most cases, the constant isn't even needed:
If a plugin/theme has installed PHPUnit + the Polyfills via Composer and includes the vendor/autoload.php file ahead of loading the WP Core bootstrap, the polyfills autoload file will already have been loaded and registered, so the constant is not needed.

This also applies to to the situation where the Polyfills + PHPUnit are installed via setup-php, as in that case, both will be installed in the Composer global directory and PHPUnit will automatically call the vendor/autoload.php file for Composer global as explained in ​https://github.com/Yoast/PHPUnit-Polyfills/#q-how-do-i-run-my-tests-when-the-library-is-installed-via-the-github-actions-setup-php-action-

So with that in mind: what problem will this solve ?

Also see: ​https://make.wordpress.org/core/2021/09/27/changes-to-the-wordpress-core-php-test-suite/

I'll leave some comments on the PR in a moment.

Then it will be easier for users to use their own PHPUnit in tools input of setup-php.

The thing is: users shouldn't use their own PHPUnit via tools in setup-php.

There's two reasons for that:

1. Performance: they will be installing PHPUnit twice in that case, once as a PHAR via tools and once via composer global as it automatically gets installed with the PHPUnit Polyfills.
2. The Polyfills are designed to be used with the most appropriate PHPUnit version for the PHP version tests are being run on. When a user requires a specific PHPUnit PHAR via tools, they are likely to create a mismatch, which can cause all sorts of problems when running the tests.

I just saw issue shivammathur/setup-php/#531 in the setup-php repo. Is this PR in response to that ticket ? Would you like me to respond to the user in that ticket ?

@jrfnl
Yes, I understand that the most optimal way would be to add phpunit-polyfills and remove phpunit from tools input and use the PHPUnit version that phpunit-polyfills sets.

But there are existing working workflows that set the PHPUnit versions in the matrix and it would be easier for them to just add phpunit-polyfills to the tools input and it just works again.
For example the workflow in ​https://github.com/shivammathur/setup-php/issues/529 was breaking when running with phpunit-polyfills's PHPUnit versions and most likely would have required refactoring the tests.

So this would make my life easier as it would give people the choice.

@jrfnl
Let me know what you think.
If this will increase the maintenance burden on your end due to version mismatches, I will close this.

But there are existing working workflows that set the PHPUnit versions in the matrix and it would be easier for them to just add phpunit-polyfills to the tools input and it just works again.

Except that's not true.

For example the workflow in ​shivammathur/setup-php#529 was breaking when running with phpunit-polyfills's PHPUnit versions and most likely would have required refactoring the tests.

If they continue to run their integration tests - WITHOUT making the required changes as per the ​WP Make Core blogpost -, their tests are _inherently_ broken now as the typical fixtures methods which they inherit from the WP test suite will no longer run when testing against WP 5.9/trunk/master, UNLESS they make the changes.

The changes aren't huge or anything, they are actually quite straight-forward to make, but they MUST be made for their integration tests to continue working.

This is inherit to the choice they made to base their tests on the WP native test suite, which means that if the WP Core test suite changes, their tests will also need to be updated.

While they may not see any tests breaking now, they will soon enough, once they start testing against WP 5.9, or else, they may start running into weird race-conditions in their tests due to the fixtures no longer running.

So this would make my life easier as it would give people the ability to choose the PHPUnit version.

I understand and appreciate that, but you shouldn't be the person who should need to solve their workflow problems, when they are not following the very clear instructions which have been publicly posted for them to follow for over two months.

IMO, point them to the blogpost on Make and tell them to follow the instructions given there or, if needs be, ping me in an issue and I will put them right.

I very much realize that plugins/themes which want to test against a range of WP versions need to make some accommodations in their workflows for the PHPUnit versions which WP allows for (which have only now been widened in WP 5.9 to encompass the full range), but there are other ways to do that.

If they continue to run their integration tests - WITHOUT making the required changes as per the WP Make Core blogpost -, their tests are inherently broken now as the typical fixtures methods which they inherit from the WP test suite will no longer run when testing against WP 5.9/trunk/master, UNLESS they make the changes.

Thanks for the detailed response, I wasn't aware of this.
From now on, I will point to the blog post for any issues regarding this. Closing the PR.

@jrfnl Let me know what you think. If this will increase the maintenance burden on your end due to version mismatches, I will close this.

@shivammathur It's not so much about the maintenance burden as it is about people _doing it wrong_ and this solution would allow them to continue to _do it wrong_.

What the best way to solve it is, depends on the WP versions they want to support/test against:

• If only testing against the current develop version or WP 5.9 and higher: by all means, they can safely use tools: phpunit-polyfills and things will just work.
• If they also want to test against older WP versions - WP 5.2 - 5.8 -, they will need to install an appropriate PHPUnit version within the PHPUnit 5.x-7.x range specifically for those builds and should not try to run the tests against PHP 8.1.

The _recommended_ way to set up their tests in that case is to use a project local Composer install with PHPUnit + the polyfills and a PHPUnit version constraint for 5.x-7x and to on-the-fly remove the PHPUnit require for running against WP 5.9+ (the polyfills will then kick in and install the more appropriate PHPUnit version).
They still _could_ use the tools: phpunit-polyfills if they insist. In that case, they should run a `composer global phpunit/phpunit:"${{ matrix.phpunit-version}}" command to switch the PHPUnit version (as PHPUnit will be globally installed via Composer already).

• If they also still want to test against yet older WP version, things get even more complicated and I'd recommend for them to use the WP Test Utils (also mentioned in the blogpost) as there are some more complications to get round in that case.

Other than that, the version switching as described above would still apply.

P.S.: just noticed you closed the PR while I was still writing this. Please also feel free to point people to this PR as I imagine the explanations I've given here may help them as well.