Tests/Script Loader: Run build step in PHP Unit test actions.

[peterwilsoncc]

The files src/wp-includes/assets/script-loader-react-*.php are created during the NPM build and included within the script loader.

Unlike src/wp-includes/assets/script-loader-packages.min.php, these files are not included in the commit when updating WordPress packages.

As a result the PHP Unit actions are now throwing warnings due to the missing files:

Warning: include(/var/www/src/wp-includes/assets/script-loader-react-refresh-entry.php): failed to open stream: No such file or directory in /var/www/src/wp-includes/script-loader.php on line 243
Warning: include(): Failed opening '/var/www/src/wp-includes/assets/script-loader-react-refresh-entry.php' for inclusion (include_path='.:/usr/local/lib/php') in /var/www/src/wp-includes/script-loader.php on line 243
Warning: include(/var/www/src/wp-includes/assets/script-loader-packages.php): failed to open stream: No such file or directory in /var/www/src/wp-includes/script-loader.php on line 278
Warning: include(): Failed opening '/var/www/src/wp-includes/assets/script-loader-packages.php' for inclusion (include_path='.:/usr/local/lib/php') in /var/www/src/wp-includes/script-loader.php on line 278
Warning: Invalid argument supplied for foreach() in /var/www/src/wp-includes/script-loader.php on line 280

To resolve this, an npm run build:dev will need to be included in the phpunit actions.

Build WP in the source directory prior to running the PHP Unit tests.

This is to account for the generated files in src/wp-includes/assets.

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

Alternative approach to PR #4161.

Instead of building the assets during the build, only attempt to load the development assets if the dependency files exist.

This idea may be entirely broken if SCRIPT_DEBUG absolutely can not get by without the files.

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

[peterwilsoncc]

I've linked two pull request to this ticket, depending on what is considered the best approach.

• ​Option one: Run npm run build:dev as part of the PHP Unit GitHub actions.
• ​Option two: As the error only occurs if SCRIPT_DEBUG === true, then it may be possible to skip registering the scripts if the dependency files are not loaded. This approach may be a non-starter if other assets assume the presence of the development scripts if SCRIPT_DEBUG is true.

[costdev]

Curious if we can reduce the build steps needed here so that it doesn't take quite as long to run CI with a full build:dev going forward.

Is there a particular (set of) step(s) in the build process that would solve this?

I appreciate that a full build could prevent potential future issues elsewhere, although it may also be useful to know what these issues were in case there's a more appropriate solution for them on a case-by-case basis.

[TobiasBg]

I've also been getting this in my plugin's unit tests for a few months now. So a solution not just in the WP core build/test process would be nice.

There's also something similar being talked about at https://core.trac.wordpress.org/ticket/56615#comment:15

[desrosj]

Thanks @TobiasBg. I thought I had seen this somewhere else but couldn't recall it late last night. That also explains why it's happening in the 6.1 branch but not the ​6.0 one.

I'm leaning towards closing this as a duplicate since #56615 is looking at adjusting the underlying commit that introduced the issue.

[desrosj]

Sorry, I added the wrong keyword previously.

[peterwilsoncc]

@desrosj I was able to create just the files needed running the following npm run -- grunt --dev webpack:prod; npm run -- grunt --dev webpack:dev

Unfortunately they're the longest running tasks.

It might be possible to get away with only webpack:dev for the GH actions as the test suite has SCRIPT_DEBUG enabled.

[azaozz]

I think this is "broken" on two different levels :(

1. Scripty-loader is most of all a list of all available scripts and stylesheets. It is wrong to juggle what is registered when. All files that are in WP should always be registered. I know it's been getting messier lately, hoping it can get fixed in 6.3.

2. Automated testing should always be done on the production files, i.e. in /build and without SCRIPT_DEBUG and probably without WP_DEBUG. If testing of the non-production files is needed, it can be done in another run of the test tools. Testing non-production files (in /src and/or with SCRIPT_DEBUG, etc.) is just wrong as it may miss several kinds of bugs and errors, sometimes severe (has happened in the past).

We are very close to RC but the second point can still be fixed before the 6.2 release.

It would be good to cross-check with ​https://github.com/WordPress/wordpress-develop/pull/3340. I remember it was super complex to set the codebase in the state that works with unit tests without running the build.

[SergeyBiryukov]

Replying to azaozz:

2. Automated testing should always be done on the production files, i.e. in /build and without SCRIPT_DEBUG and probably without WP_DEBUG. If testing of the non-production files is needed, it can be done in another run of the test tools. Testing non-production files (in /src and/or with SCRIPT_DEBUG, etc.) is just wrong as it may miss several kinds of bugs and errors, sometimes severe (has happened in the past).

Hmm, #51734 had some good arguments for running PHPUnit tests from src instead of build:

1. Running a build is slow. It copies all the files and builds, validates, and minifies all the CSS and JS. None of this should be necessary for PHP testing.
2. A developer iterating on a patch in the source file has no way of knowing that their file is not actually being tested when running the tests, unless they run the build each time or start and run the file watcher. This is an easy step to forget.
3. PHP errors display a stack trace from build instead src.
4. Breakpoint debugging isn't fun as it also uses the stack trace from build instead of src.
5. Any test which requires a build step is not a unit test and should be tested using other means, for example as a validation step during the build process.

As someone who runs PHP tests locally on pretty much a daily basis, I find it very helpful that the build step is currently not required for that, saves a lot of time :)

[azaozz]

Replying to SergeyBiryukov:

#51734 had some good arguments for running PHPUnit tests from src instead of build
...
I find it very helpful that the build step is currently not required for that, saves a lot of time :)

Yep, I agree, PHPUnit testing in /src is faster and more convenient for developers. But it also has one big problem: testing from /src does not test some of the production (built) files. That makes it unreliable.

So perhaps running PHPUnit tests from /src may be considered "developer convenience" but imho should not be considered acceptable for production testing of WordPress. I.e. tests that run after a commit should probably be testing the production files, not the "development (partial) build" in /src.

Thinking there should be an easy way to tell PHPUnit where to test from cli. Seems pretty easy to add couple more shortcuts to package.json. Then fix grunt test as currently it seems to be super slow and doing a lot of pointless stuff like running grunt build every time even when not needed and testing unchanged files, etc. Also perhaps stuff like LOCAL_DIR="build" npm run env:start can be documented better or automated.

And of course it won't hurt to keep an eye on the "next-gen unit testing". Seems it's coming faster than expected :) ​https://www.tabnine.com/blog/ai-unit-test-generation/

file_exists() seems like a good idea for WP 6.2 however don't think this is a good way to handle either script-loader or (PHP) testing in the longer run.

• There is no good reason for the $development_scripts to be missing in /src. This goes against the main design/purpose of script-loader.
• Production level testing should always be done in /build. Building and testing /src is perhaps a "developer convenience" but is not suitable for "final (production) testing".

See: ​https://github.com/WordPress/wordpress-develop/pull/3340 and https://core.trac.wordpress.org/ticket/57844#comment:13.

This PR doesn't look it fixes the issues on line 278 / 280.

Warning: include(/var/www/src/wp-includes/assets/script-loader-react-refresh-entry.php): failed to open stream: No such file or directory in /var/www/src/wp-includes/script-loader.php on line 243
Warning: include(): Failed opening '/var/www/src/wp-includes/assets/script-loader-react-refresh-entry.php' for inclusion (include_path='.:/usr/local/lib/php') in /var/www/src/wp-includes/script-loader.php on line 243
Warning: include(/var/www/src/wp-includes/assets/script-loader-packages.php): failed to open stream: No such file or directory in /var/www/src/wp-includes/script-loader.php on line 278
Warning: include(): Failed opening '/var/www/src/wp-includes/assets/script-loader-packages.php' for inclusion (include_path='.:/usr/local/lib/php') in /var/www/src/wp-includes/script-loader.php on line 278
Warning: Invalid argument supplied for foreach() in /var/www/src/wp-includes/script-loader.php on line 280

This PR doesn't look it fixes the issues on line 278 / 280.

Thanks for catching that Jonny, I've pushed ​https://github.com/WordPress/wordpress-develop/pull/4162/commits/719276e22b6490a00a3b2a94e19951361b906974 to add a similar check to wp_default_packages_scripts().

[spacedmonkey]

@peterwilsoncc I would love to see this change backported a couple of releases. I come up against this issue on older tags as well. ​See.

[spacedmonkey]

Marked as ready to commit.

[costdev]

This ticket was discussed during the bug scrub. As we're about to enter RC, moving this ticket to 6.3 and backporting to the 6.2 branch can be discussed there.

[SergeyBiryukov]

Replying to azaozz:

Replying to SergeyBiryukov:

#51734 had some good arguments for running PHPUnit tests from src instead of build
...
I find it very helpful that the build step is currently not required for that, saves a lot of time :)

Yep, I agree, PHPUnit testing in /src is faster and more convenient for developers. But it also has one big problem: testing from /src does not test some of the production (built) files. That makes it unreliable.

So perhaps running PHPUnit tests from /src may be considered "developer convenience" but imho should not be considered acceptable for production testing of WordPress. I.e. tests that run after a commit should probably be testing the production files, not the "development (partial) build" in /src.

Should testing of the production files perhaps be considered integration or e2e testing (and be a part of the e2e test suite, for example), rather than unit testing?

My understanding is that unit tests are supposed to be mostly focused on individual functions or classes:

• ​Unit testing vs integration testing
• ​The Practical Test Pyramid

Trac ticket:

[peterwilsoncc]

Removing the commit keyword for now as I think this still needs a bit of work.

Of the linked pull requests, I prefer ​#4586 as it makes use of caching to avoid slowing down each run of PHPUnit.

[westonruter]

This should also allow us to remove the instances of self::touch(() in tests, for example:

• ​https://github.com/WordPress/wordpress-develop/blob/2b931da/tests/phpunit/tests/formatting/emoji.php#L18-L19
• ​https://github.com/WordPress/wordpress-develop/blob/2b931da/tests/phpunit/tests/oembed/wpOembed.php#L22C1-L23
• ​https://github.com/WordPress/wordpress-develop/blob/2b931da/tests/phpunit/tests/blocks/register.php#L357-L358
• etc

[audrasjb]

Moving to milestone 6.4 as WP 6.3 RC3 has been released.

[swissspidy]

FWIW this also happens for e2e and performance test runs on GitHub Actions, where npm run build is followed by npm run env:start and npm run env:install.

It's in the npm run env:install step where these warnings occur.

[peterwilsoncc]

@zieladam is currently working on a ​pull request to create a built artifact so I'm going to bump this pending the progress on Adam's PR.

[chaion07]

Thanks @peterwilsoncc for reporting this. We reviewed this ticket during a recent bug-scrub session. Based on the feedback received we want to confirm the following:

1. Peter's last comment refers to Adam working on a build artifact
2. This has been done and is the asset used for WordPress Playground
3. We want to confirm and see what is coming next for this ticket.

Cheers!

Props to @costdev for the discussion

[audrasjb]

Moving to 6.7