Run wp-env start before PHP unit tests in package scripts #23797
Conversation
|
Size Change: 0 B Total Size: 1.16 MB ℹ️ View Unchanged
|
|
I don't think it makes sense to run |
|
Let's make |
noahtallen
force-pushed
the
fix/use-local-wp-env-for-php-test
branch
from
d001e47 to
f1ef953
Compare
|
@epiqueras done in #23809 Since wp-env start only takes 2 seconds to complete in most cases, I'll add it to the package script here. |
noahtallen
changed the title
Getting that in the phpunit tests. Looks suspiciously like #20180 (comment) |
package.json
Outdated
| "test-unit-php": "wp-env start && wp-env run phpunit 'phpunit -c /var/www/html/wp-content/plugins/gutenberg/phpunit.xml.dist --verbose'", | ||
| "test-unit-php-multisite": "wp-env start && wp-env run phpunit 'WP_MULTISITE=1 phpunit -c /var/www/html/wp-content/plugins/gutenberg/phpunit/multisite.xml --verbose'", |
There was a problem hiding this comment.
Aside, should we also add
| "test-unit-php": "wp-env start && wp-env run phpunit 'phpunit -c /var/www/html/wp-content/plugins/gutenberg/phpunit.xml.dist --verbose'", | |
| "test-unit-php-multisite": "wp-env start && wp-env run phpunit 'WP_MULTISITE=1 phpunit -c /var/www/html/wp-content/plugins/gutenberg/phpunit/multisite.xml --verbose'", | |
| "pretest-unit-php": "wp-env run composer install -- --no-interaction", | |
| "test-unit-php": "wp-env start && wp-env run phpunit 'phpunit -c /var/www/html/wp-content/plugins/gutenberg/phpunit.xml.dist --verbose'", | |
| "pretest-unit-php-multisite": "wp-env run composer install -- --no-interaction", | |
| "test-unit-php-multisite": "wp-env start && wp-env run phpunit 'WP_MULTISITE=1 phpunit -c /var/www/html/wp-content/plugins/gutenberg/phpunit/multisite.xml --verbose'", |
? I know that @Copons was running into that issue when first trying to npm run test-unit-php.
There was a problem hiding this comment.
I'm not familiar with this kind of things, but yes, I can confirm that npm run test-unit-php failed for me when spinning up the Docker images, and it was fixed (as suggested by @ockham) after running composter install.
There was a problem hiding this comment.
The problem with doing that is when you run test-php, then it's going to run composer install 3 times. May not be a huge issue, but it will add some additional overhead.
There was a problem hiding this comment.
Indeed 😕 Is there maybe some CLI option to make composer install smart enough to only install stuff if something has changed?
There was a problem hiding this comment.
It will detect and not install anything if there are no changes, but it still has to check. I do think it regenerates the autoload files by default unless you pass --no-autoloader. It's fairly quick, but depends on how many dependencies there are.
There was a problem hiding this comment.
True.. Could we just remove the lint check from test-php?
There was a problem hiding this comment.
If we don't end up executing composer install automatically, would it be worthwhile to add a note under ## PHP Testing in docs/contributors/testing-overview.md about running composer install before npm run test-unit-php?
Indeed. FWIW, on this branch, |
|
To back up a little bit -- I'm quite skeptical that simply running |
|
@ockham can you run that with the That's really odd. It's definitely a Linux thing since it seems to be working for those of us with macOS 🤔 wp-env start is definitely required to run test-unit-php in general (so that WordPress is configured correctly), but it sounds like there might be further issues on linux. |
|
Though to be fair, GitHub CI is running Linux, and it has been working fine 🤔 |
|
@ockham |
|
Speaking of composer, it is a lot faster to run it locally from cwd than inside wp-env. 🤔 (like the difference between a few seconds and a few minutes) |
There was a problem hiding this comment.
> gutenberg@8.5.0 test-unit-php /Users/jeremyyip/Desktop/gutenberg
> wp-env start && wp-env run phpunit 'phpunit -c /var/www/html/wp-content/plugins/gutenberg/phpunit.xml.dist --verbose'
✔ WordPress started. (in 35s 51ms)
ℹ Starting 'phpunit -c /var/www/html/wp-content/plugins/gutenberg/phpunit.xml.dist --verbose' on the phpunit container.
Starting 38d33493c96d64b36cf07d211e8bf769_mysql_1 ... done
Starting 38d33493c96d64b36cf07d211e8bf769_tests-wordpress_1 ... done
Fatal error: Uncaught Error: Class 'PHPMailer' not found in /var/www/html/wp-content/plugins/gutenberg/vendor/wp-phpunit/wp-phpunit/includes/mock-mailer.php:4
Stack trace:
#0 /var/www/html/wp-content/plugins/gutenberg/vendor/wp-phpunit/wp-phpunit/includes/bootstrap.php(87): require_once()
#1 /var/www/html/wp-content/plugins/gutenberg/phpunit/bootstrap.php(74): require('/var/www/html/w...')
#2 phar:///usr/local/bin/phpunit/phpunit/Util/FileLoader.php(57): include_once('/var/www/html/w...')
#3 phar:///usr/local/bin/phpunit/phpunit/Util/FileLoader.php(45): PHPUnit\Util\FileLoader::load('/var/www/html/w...')
#4 phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php(1058): PHPUnit\Util\FileLoader::checkAndLoad('/var/www/html/w...')
#5 phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php(863): PHPUnit\TextUI\Command->handleBootstrap('/var/www/html/w...')
#6 phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php(173): PHPUnit\TextUI\Command->handleArguments(Array)
#7 phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.p in /var/www/html/wp-content/plugins/gutenberg/vendor/wp-phpunit/wp-phpunit/includes/mock-mailer.php on line 4
✖ Command failed with exit code 255
I'm not sure if this is related to the PR (especially since it's mentioning PHPMailer), but I'm running into an exception when trying to test this branch locally.
I'm running into this exception locally on master. I'm going to spend some time debugging, but does anyone happen to have an idea of how to resolve the reference to PHPMailer?
I can confirm that executing the |
|
The tests work without the containers being currently running, but you have to have run If they are stopped, the phpunit command will still spin up what it needs to run the tests, but it won’t provision them correctly. |
That makes sense to me. wp-env start is the only command which runs |
That's a good callout. Thanks for clarifying 👍 |
|
I just tried So I guess we can close this PR? |
Ha, well I'm not sure what the next steps are here. But ideally, I would like to make sure that:
when running the phpunit tests. I think that introduces a lot of confusion (a lot of people end up asking how to get them working!) So i'd still like to do that (and update the docs), but I'm not sure how to fix the permissions error that happens when running wp-env twice in CI.... BTW, the stylesheet error you see is still pretty weird. I also don't think it's wp-env's fault, though it could be. Maybe related to whichever theme is active on your site, or maybe one of the mapped themes is not compiled? (or maybe the mapped wordpress needs to be updated and recompiled?) |
noahtallen
force-pushed
the
fix/use-local-wp-env-for-php-test
branch
from
a25b0b4 to
25cf84e
Compare
looks like I was able to fix it with a quick |
|
I'm debating whether to put I'm noticing that |
|
Cache didn't make a difference. Something about https://github.com/Dealerdirect/phpcodesniffer-composer-installer causes it to take a very long time to install |
noahtallen
force-pushed
the
fix/use-local-wp-env-for-php-test
branch
from
25cf84e to
444d684
Compare
|
cc @epiqueras @ockham I think we can move forward with what we have now and simply run Something is still wrong with the composer command, since it takes >80s to run in Docker but 2s outside of docker locally. I created an issue for that here: #23944, but I probably won't have time to look into it immediately. |
|
Could this good another look? :) |
noahtallen
force-pushed
the
fix/use-local-wp-env-for-php-test
branch
from
911e635 to
aa11386
Compare
|
These changes work well in CI ^^ Also, this fixes a linux issue and improves docs, so I'll go ahead and merge :) |
Description
If you had installed wp-env globally, the(I was wrong about this, package.json scripts will reference the local module by default without needing thetest-unit-phpscript would reference that instead of the local installation. Like many of our otherpackage.jsonscripts, we need to reference the local source code.npm runprefix)Test unit php was working correctly in CI
because it probably was able to resolve the local sourcecode since there was no global install to resolve first.(It was actually working correctly becausewp-env starthad been called before running tests.)This caused some issues for folks running php unit tests locally, particularly if wp-env start was not running yet. This resolves #23642 since we are now running wp-env start before starting the unit tests.
Once wp-env is set up once, performance will be really fast as of #23809 (about 2 seconds), so this should be fine to add here.
How has this been tested?
Locally with wp-env. I ran
npm run test-unit-phplocally and it works correctly.Screenshots
Types of changes
Bug fix.
Checklist: