The PHP tests initialize very slowly in wp-env

[azaozz]

When run in wp-env (Docker), the PHP tests take very long time (over 30 seconds) to initialize. This is especially annoying/time wasting when running them multiple times.

Not sure if Docker or Composer is the cause, but I see this:

> WordPress@6.2.0 test:php /dev/wp/trunk
> node ./tools/local-env/scripts/docker.js run -T php composer update -W && node ./tools/local-env/scripts/docker.js run php ./vendor/bin/phpunit

Creating trunk_php_run ... done
.................................... (waits here for about 30-35 seconds)
Loading composer repositories with package information
Info from https://repo.packagist.org: #StandWithUkraine
Updating dependencies
Nothing to modify in lock file
Installing dependencies from lock file (including require-dev)
Nothing to install, update or remove
Generating autoload files
25 packages you are using are looking for funding.
Use the `composer fund` command to find out more!
Creating trunk_php_run ... done
Installing...
Running as single site... To run multisite, use -c tests/phpunit/multisite.xml
..... (rest of the PHPUnit output)

The problem seems to be that Composer is run every time to re-check the dependencies, i.e. the script there does the equivalent of npm i every time before it starts PHPUnit. I understand the desire to keep things updated, but this is not productive.

[azaozz]

Seems an easy fix here would be to split the "Composer install" part from the actual "run PHPUnit" part in npm run test:php. Something like:

"test:php-i": "node ./tools/local-env/scripts/docker.js run -T php composer update -W && node ./tools/local-env/scripts/docker.js run php ./vendor/bin/phpunit",
"test:php": "node ./tools/local-env/scripts/docker.js run php ./vendor/bin/phpunit",

A better way would be to have an equivalent of a lock file and run Composer only once per week.

[azaozz]

Another thing is that if a PHPUnit test fails, the (npm/Docker) script fails too. Seems not a big problem but needs fixing nevertheless. Error is:

ERROR: 1
child_process.js:836
    throw err;
    ^

Error: Command failed: docker-compose run php ./vendor/bin/phpunit

[hellofromTonya]

Given that RC1 is fast approaching (next week) and this needs more discussion, moving it to 6.3. But if can be committed at any time when ready.

[swissspidy]

+1 to this. I always manually remove this part during development because it's so wasteful.

[johnbillion]

This was intentionally added in #53945 but I'd love to get rid of it

[desrosj]

I think this is still important to have because of the edge cases detailed in #53945.

What about moving this to the env:start command? That would handle the scenarios that ticket aimed to solve and would not run composer update every time tests run. Arguably that's the more appropriate time to run that command anyway since npm run env:restart is required if the desired PHP version is changed. It also seems to run much faster.

If the consensus is to remove it I'm not opposed. But we'll need to provide better documentation around when to run the command and what the indicators are when it needs to be run.

[hellofromTonya]

6.4 RC1 is happening in a couple of hours and trunk will be branched. Moving this to 6.5.

[swissspidy]

#61350 was marked as a duplicate.

[desrosj]

Because we've passed the branching point for 6.6, there's no real need to commit and backport this. I'm going to commit 57189.diff​ to try during the 6.7 cycle.

It still runs the composer update command, just much less often. Running once per session while using the container is much better than every time the test script is run. It also still accomplishes the goal of eliminating the need for another explicit step in the already complicated process of getting the local Docker environment up and running, potentially making it easier to get up and running working on tests.

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

[desrosj]

There is one unintended consequence of moving composer update to env:start: the command runs even when you don't require the Composer packages.

One scenario this comes up is in the E2E test workflow. But even then it only seems to add 5-10 seconds to the corresponding step (assuming network speeds are identical pulling containers, which is unlikely). It seems like this could be an OK trade off, though.

I don't really have any strong feelings here other than "we should try to take care of this so the contributor does not have to worry about it", so waiting to commit so others can chime in.

[peterwilsoncc]

Replying to desrosj:

There is one unintended consequence of moving composer update to env:start: the command runs even when you don't require the Composer packages.

One scenario this comes up is in the E2E test workflow. But even then it only seems to add 5-10 seconds to the corresponding step (assuming network speeds are identical pulling containers, which is unlikely). It seems like this could be an OK trade off, though.

As it's a dev environment, I think it's fine to run them on env:start. That will ensure that phpcs is available for developers.

I know having xDebug enabled can slow the php tests down a lot, much more than a few seconds, so it may be worth looking at that too.

[desrosj]

I know having xDebug enabled can slow the php tests down a lot, much more than a few seconds, so it may be worth looking at that too.

xDebug should already be disabled by default, and it should only be enabled when running the xdebug group. The only other time I'm aware of it being enabled is when generating the code coverage report

I'll leave this a few more days for any further feedback, but will circle back to commit 57189.diff​ later this week if there's no objection.

[chaion07]

Thanks @azaozz for reporting this. We reviewed this Ticket during a recent bug-scrub session. Based on the feedback received we can confirm that so far there are no objections and the Ticket can move forward as expected. Thanks.

Cheers!

[desrosj]

There was some discussion on the PR around potentially moving the command to the env:install task instead.

@hellofromTonya posed it as a potential optimization to the suggested approach and not an objection to using env:start, so I think it's reasonable to move forward with the PR as is. Here were my thoughts around using env:install for easy reference since the comment did not make it's way over to Trac.

Once someone runs env:install, they won't be re-running that command unless they are recreating their environment from scratch. When this is on env:start or test:php, composer update is run ensuring the latest versions are always installed before running tests.

[desrosj]

In 59231:

Build/Test Tools: Move composer update to env:start.

Currently, composer update is run whenever test:php is called to ensure the latest versions of yoast/phpunit-polyfills and other dependencies are always installed when running the PHPUnit test suite.

For contributors using the local Docker environment to run tests during development, this is unnecessary and can often result in a 30+ second delay every time test:php is called.

This moves the command to env:install, reducing the number of times composer update is run from many to once. Since the environment needs to be started in order to run tests, env:install will still confirm that the latest versions of required dependencies are installed and available prior to running the test suite.

Props azaozz, swissspidy, johnbillion, peterwilsoncc, hellofromtonya.
Fixes #57189.

Merged in https://core.trac.wordpress.org/changeset/59231.