Env: correct multisite support #22613

noahtallen · 2020-05-25T23:36:13Z

Description

So it turns out that wp-env isn't really doing multisite tests right now. After some debugging by @TimothyBJacobs, we discovered that our wp-config.php requires wp-settings.php before MULTISITE is defined to true. This means that wp-phpunit can't redefine that constant to true. Normally, wp-phpunit would define the constant to true if there is an existing MULTISITE env variable and then load the wp-settings.php file.

Basically, wp-settings.php is being loaded twice: once by us and once by wp-phpunit. We can solve the multisite issue (and other issues, I'm sure) by not loading it ourselves when running wp-phpunit. I've done this by creating a copy of wp-config.php which does not load wp-settings.php when wordpress is initialized by wp-env.

One nice bonus is that this is also the cause of our "redeclared constant" errors. Before this PR, you'd see 3-4 of those when running phpunit, and now there are none. This makes a lot of sense if these files have been loaded twice!

Unfortunately, after fixing this, I am getting the following error running multisite tests. This error does not appear for single site test runs.

$ npm run test-unit-php-multisite

> gutenberg@8.2.0-rc.1 test-unit-php-multisite /Users/noah.allen/source/gutenberg
> wp-env run phpunit 'WP_MULTISITE=1 phpunit -c /var/www/html/wp-content/plugins/gutenberg/phpunit.xml.dist'

✖ Error while running docker-compose command.
Installing...
Installing network...
Running as multisite...

wp_die called
Message : <h1>Error establishing a database connection</h1>
Title : Database Error

wp_die called
Message : <h1>Error establishing a database connection</h1><p>If your site does not display, please contact the owner of this network. If you are the owner of this network please check that MySQL is running properly and all tables are error free.</p><p><strong>Could not find site <code>localhost:8889</code>.</strong> Searched for table <code>wp_blogs</code> in database <code>tests-wordpress</code>. Is that right?</p><p><strong>What do I do now?</strong> Read the <a href="https://wordpress.org/support/article/debugging-a-wordpress-network/" target="_blank">bug report</a> page. Some of the guidelines there may help you figure out what went wrong. If you&#8217;re still stuck with this message, then check that your database contains the following tables:</p><ul><li>wp_users</li><li>wp_usermeta</li><li>wp_blogs</li><li>wp_blogmeta</li><li>wp_signups</li><li>wp_site</li><li>wp_sitemeta</li><li>wp_registration_log</li></ul>
Title : Error establishing a database connection
Args:
	 response : 500

How has this been tested?

Made sure that wp-env start works after wp-env destroy, and that it works running wp-env start multiple times in a row.
phpunit tests should pass locally and in CI.

Screenshots

Types of changes

bug fix

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR.

github-actions · 2020-05-25T23:41:42Z

Size Change: +13.9 kB (1%)

Total Size: 1.12 MB

Filename	Size	Change
`build/a11y/index.js`	1.14 kB	+1 B
`build/annotations/index.js`	3.62 kB	+1 B
`build/api-fetch/index.js`	3.4 kB	+2 B (0%)
`build/autop/index.js`	2.83 kB	-1 B
`build/block-directory/index.js`	6.75 kB	+269 B (3%)
`build/block-directory/style-rtl.css`	892 B	+105 B (11%)	⚠️
`build/block-directory/style.css`	892 B	+105 B (11%)	⚠️
`build/block-editor/index.js`	106 kB	+502 B (0%)
`build/block-editor/style-rtl.css`	11.4 kB	+49 B (0%)
`build/block-editor/style.css`	11.4 kB	+49 B (0%)
`build/block-library/editor-rtl.css`	7.87 kB	+264 B (3%)
`build/block-library/editor.css`	7.88 kB	+265 B (3%)
`build/block-library/index.js`	126 kB	+7.2 kB (5%)	🔍
`build/block-library/style-rtl.css`	7.69 kB	+9 B (0%)
`build/block-library/style.css`	7.68 kB	+5 B (0%)
`build/block-serialization-default-parser/index.js`	1.88 kB	+1 B
`build/block-serialization-spec-parser/index.js`	3.1 kB	-1 B
`build/blocks/index.js`	48.1 kB	+20 B (0%)
`build/components/index.js`	193 kB	+3.61 kB (1%)
`build/components/style.css`	19.5 kB	+4 B (0%)
`build/compose/index.js`	9.33 kB	+10 B (0%)
`build/core-data/index.js`	11.4 kB	+11 B (0%)
`build/data/index.js`	8.46 kB	+30 B (0%)
`build/date/index.js`	5.47 kB	+3 B (0%)
`build/edit-navigation/index.js`	8.25 kB	+365 B (4%)
`build/edit-navigation/style-rtl.css`	878 B	+21 B (2%)
`build/edit-navigation/style.css`	876 B	+20 B (2%)
`build/edit-post/index.js`	302 kB	-42 B (0%)
`build/edit-site/index.js`	15 kB	+872 B (5%)	🔍
`build/edit-widgets/index.js`	8.83 kB	-48 B (0%)
`build/editor/index.js`	44.7 kB	+76 B (0%)
`build/element/index.js`	4.65 kB	+1 B
`build/format-library/index.js`	7.72 kB	+8 B (0%)
`build/html-entities/index.js`	621 B	-1 B
`build/keyboard-shortcuts/index.js`	2.52 kB	+4 B (0%)
`build/keycodes/index.js`	1.94 kB	+1 B
`build/list-reusable-blocks/index.js`	3.12 kB	-1 B
`build/media-utils/index.js`	5.3 kB	+8 B (0%)
`build/notices/index.js`	1.79 kB	+1 B
`build/nux/index.js`	3.4 kB	+1 B
`build/plugins/index.js`	2.56 kB	-3 B (0%)
`build/primitives/index.js`	1.5 kB	+1 B
`build/redux-routine/index.js`	2.85 kB	-2 B (0%)
`build/rich-text/index.js`	14.8 kB	+13 B (0%)
`build/server-side-render/index.js`	2.68 kB	+5 B (0%)
`build/url/index.js`	4.06 kB	+39 B (0%)
`build/viewport/index.js`	1.85 kB	+10 B (0%)
`build/wordcount/index.js`	1.17 kB	+1 B

ℹ️ View Unchanged

Filename	Size	Change
`build/blob/index.js`	620 B	0 B
`build/block-library/theme-rtl.css`	684 B	0 B
`build/block-library/theme.css`	686 B	0 B
`build/components/style-rtl.css`	19.5 kB	0 B
`build/data-controls/index.js`	1.29 kB	0 B
`build/deprecated/index.js`	771 B	0 B
`build/dom-ready/index.js`	568 B	0 B
`build/dom/index.js`	3.11 kB	0 B
`build/edit-post/style-rtl.css`	5.43 kB	0 B
`build/edit-post/style.css`	5.43 kB	0 B
`build/edit-site/style-rtl.css`	2.96 kB	0 B
`build/edit-site/style.css`	2.96 kB	0 B
`build/edit-widgets/style-rtl.css`	2.4 kB	0 B
`build/edit-widgets/style.css`	2.4 kB	0 B
`build/editor/editor-styles-rtl.css`	425 B	0 B
`build/editor/editor-styles.css`	428 B	0 B
`build/editor/style-rtl.css`	4.26 kB	0 B
`build/editor/style.css`	4.27 kB	0 B
`build/escape-html/index.js`	733 B	0 B
`build/format-library/style-rtl.css`	502 B	0 B
`build/format-library/style.css`	502 B	0 B
`build/hooks/index.js`	2.13 kB	0 B
`build/i18n/index.js`	3.56 kB	0 B
`build/is-shallow-equal/index.js`	711 B	0 B
`build/list-reusable-blocks/style-rtl.css`	226 B	0 B
`build/list-reusable-blocks/style.css`	226 B	0 B
`build/nux/style-rtl.css`	616 B	0 B
`build/nux/style.css`	613 B	0 B
`build/priority-queue/index.js`	789 B	0 B
`build/shortcode/index.js`	1.7 kB	0 B
`build/token-list/index.js`	1.28 kB	0 B
`build/warning/index.js`	1.14 kB	0 B

_{compressed-size-action}

epiqueras · 2020-05-25T23:56:38Z

Have you tried just hardcoding MULTISITE in the original wp-config.php instead of creating a copy that doesn't load settings?

noahtallen · 2020-05-26T00:55:01Z

Have you tried just hardcoding MULTISITE in the original wp-config.php

Yeah, I also think that would solve the original problem, but we probably wouldn’t want it turned on all the time. The reason we went with this route is that there could be other things that should be configured before loading settings, so it might be hard to pick all of those out by hand to define early. Additionally, loading settings twice causes all of the redeclared constant warnings you see running phpunit in wp-env. We would still probably not want to load the file twice

noahtallen · 2020-05-26T01:15:03Z

Note that the multisite tests are failing in this PR even though travis is green. This is because the phpunit command exits with 0 even though the database connection failed.

noahtallen · 2020-05-26T01:26:34Z

After some debugging it looks like wp_die is called at require_once ABSPATH . '/wp-settings.php'; in wp-phpunit's bootstrap.php.

noisysocks · 2020-05-26T01:31:07Z

Could this be fixed in wp-phpunit? I'm probably misunderstanding something, but my first impression is that it's strange that one has to deviate from the canonical¹ way to write a wp-config.php file in order to get tests to work.

¹ In that it's what wp-config-sample.php tells you to do.

noahtallen · 2020-05-26T01:34:01Z

@noisysocks I think this is a shortcoming of wp-phpunit, but it's an explicit one. It has us set the path to our own config file, and then it loads our config file in its own config file

noahtallen · 2020-05-26T01:35:03Z

see the "why" section here: https://github.com/wp-phpunit/docs#overview

WP PHPUnit solves this by including this file in the package source which acts as a kind of proxy to your "real" tests config file, which can be wherever you wish.

noahtallen · 2020-05-26T01:38:10Z

Could this be fixed in wp-phpunit?

I'm also conflicted here because so much about phpunit is tied to the environment (wp-env), but wp-env's primary goal isn't to support 3rd party deps like phpunit. To make it straightforward to run phpunit, we have to modify wp-env, but that means accepting phpunit under wp-env's umbrella even though it's currently a composer dependency of the Gutenberg plugin.

TimothyBJacobs · 2020-05-26T01:45:43Z

my first impression is that it's strange that one has to deviate from the canonical1 way to write a wp-config.php file in order to get tests to work.

WP also ships with a separate wp-tests-config-sample.php that is somewhat different from the plain wp-config.php format.

wp-env's primary goal isn't to support 3rd party deps like phpunit.

I think wp-env wouldn't have to work so hard if WordPress played with phpunit in a more standard way like other projects do. But because it doesn't, I think it isn't so much wp-env supporting the 3rd party tool phpunit, but rather supporting the first-party tool of the WordPress unit test suite if that makes sense.

noisysocks · 2020-05-26T02:05:32Z

see the "why" section here: https://github.com/wp-phpunit/docs#overview

Thanks! That's helpful 🙂

To make it straightforward to run phpunit, we have to modify wp-env, but that means accepting phpunit under wp-env's umbrella even though it's currently a composer dependency of the Gutenberg plugin.

Yeah... are we conflating Gutenberg and wp-env? Gutenberg depends on wp-phpunit and wp-env. wp-env depends on phpunit. wp-env should not depend on wp-phpunit. Gutenberg should invoke tests by running WP_PHPUNIT__TESTS_CONFIG=phpunit/wp-tests-config.php wp-env run phpunit. phpunit/wp-tests-config.php can be written with the assumption that the test environment is using the default wp-env port etc.

noisysocks · 2020-05-26T02:06:46Z

I think wp-env wouldn't have to work so hard if WordPress played with phpunit in a more standard way like other projects do.

It's not really relevant to this PR, but I'm curious what changes to WordPress would make it play nicer with phpunit? We have the power to change Core 🙂

noahtallen · 2020-05-26T02:10:49Z

wp-env should not depend on wp-phpunit

why not? As we've seen already, setting up phpunit is a pain, and it's good for us to support unit-tested code in the WordPress ecosystem. It'd be great to make it less of a pain, IMO. I just wonder if wp-env should be trying to absorb this complexity since the phpunit setup is so closely tied to the other configuration code in wp-env

TimothyBJacobs · 2020-05-26T02:41:47Z

Pushed a commit to set the additional test constants that multisite needs.

It's not really relevant to this PR, but I'm curious what changes to WordPress would make it play nicer with phpunit?

I think a big thing is that wp-config.php has side effects which means we need to go to all this trouble. Also that all these constants need to be set which sort of necessitates a separate tests config file.

noahtallen · 2020-05-26T03:20:44Z

Hm, still seeing the database connection here: https://travis-ci.com/github/WordPress/gutenberg/jobs/339864292

TimothyBJacobs · 2020-05-26T03:21:53Z

Yeah it's strange. Before that commit, the tests blew up entirely for me, and after that commit they work. But that seemed to have no effect in travis?

.travis.yml

epiqueras · 2020-05-26T15:48:21Z

Yeah, I also think that would solve the original problem, but we probably wouldn’t want it turned on all the time.

I was just wondering if that approach at least gets the multisite tests to pass.

noahtallen · 2020-05-26T17:17:34Z

if that approach at least gets the multisite tests to pass.

Theoretically, it shouldn't change behavior, since wp-phpunit does define the constant if it exists as an environment variable. We've confirmed that behavior works, but it's worth a shot

noahtallen · 2020-05-26T18:49:37Z

I think a good first step here is to make sure the multisite test fails if:

it's not actually multisite
the database connection fails

If either of these fail, we currently don't know about it without looking at the logs since travis passes.

TimothyBJacobs · 2020-05-26T19:50:04Z

AFAICT, we are at the point that the multisite tests are actually passing and working on multisite. The issue is the db error output which I think is happening because it isn't able to find the right current site.

noahtallen · 2020-05-26T21:15:15Z

Another oddity is that if I add a wp_die call in includes/install.php, it does fail the test run for me locally. It also fails if wp_die is added to a random test. So I'm not sure where this one is happening that it avoids the normal exception behavior

noahtallen · 2020-05-26T21:31:41Z

the current failure is a flakey e2e test, not php. Don't let that fool you into thinking multisite php tests are working because they aren't

TimothyBJacobs · 2020-06-03T19:19:52Z

Was finally able to get an error message to appear

Error populating network: You must provide a valid email address.

We did add this constant though in packages/env/lib/config.js.

If you are missing WP_TESTS_EMAIL, populate_network will fail and it can be hard to debug. As populate_network can return a wp_error object, we can detect that and display the error to a user. See: WordPress/gutenberg#22613 Fixes: #50251 Props: TimothyBlynJacobs git-svn-id: https://develop.svn.wordpress.org/trunk@47904 602fd350-edb4-49c9-b593-d223f7449a82

If you are missing WP_TESTS_EMAIL, populate_network will fail and it can be hard to debug. As populate_network can return a wp_error object, we can detect that and display the error to a user. See: WordPress/gutenberg#22613 Fixes: #50251 Props: TimothyBlynJacobs Built from https://develop.svn.wordpress.org/trunk@47904 git-svn-id: http://core.svn.wordpress.org/trunk@47678 1a063a9b-81f0-0310-95a4-ce76da25c4cd

If you are missing WP_TESTS_EMAIL, populate_network will fail and it can be hard to debug. As populate_network can return a wp_error object, we can detect that and display the error to a user. See: WordPress/gutenberg#22613 Fixes: #50251 Props: TimothyBlynJacobs Built from https://develop.svn.wordpress.org/trunk@47904 git-svn-id: https://core.svn.wordpress.org/trunk@47678 1a063a9b-81f0-0310-95a4-ce76da25c4cd

noahtallen · 2020-06-03T23:58:04Z

cc @epiqueras @noisysocks we have the tests passing now. Just had to copy the test config values to the override file! should be good for a review 👍

epiqueras · 2020-06-04T00:16:02Z

Great work here!

If you are missing WP_TESTS_EMAIL, populate_network will fail and it can be hard to debug. As populate_network can return a wp_error object, we can detect that and display the error to a user. See: WordPress/gutenberg#22613 Fixes: #50251 Props: TimothyBlynJacobs git-svn-id: https://develop.svn.wordpress.org/trunk@47904 602fd350-edb4-49c9-b593-d223f7449a82

If you are missing WP_TESTS_EMAIL, populate_network will fail and it can be hard to debug. As populate_network can return a wp_error object, we can detect that and display the error to a user. See: WordPress/gutenberg#22613 Fixes: #50251 Props: TimothyBlynJacobs git-svn-id: https://develop.svn.wordpress.org/trunk/tests/phpunit/includes@47904 602fd350-edb4-49c9-b593-d223f7449a82

noahtallen added [Type] Bug An existing feature does not function as intended [Package] Env /packages/env labels May 25, 2020

noahtallen requested review from noisysocks, TimothyBJacobs and epiqueras May 25, 2020 23:36

noahtallen self-assigned this May 25, 2020

noahtallen commented May 26, 2020

View reviewed changes

.travis.yml Show resolved Hide resolved

noahtallen force-pushed the fix/phpunit-multisite-and-redefined-const-errors branch from ff2ba0c to ed4471f Compare May 26, 2020 18:50

TimothyBJacobs added 4 commits June 3, 2020 11:31

Dump some more tables.

a73077d

Dump all installed tables

a538115

Fix using forked package

e5f62d4

More wp-phpunit adjustments

066710e

TimothyBJacobs added 6 commits June 3, 2020 15:23

More wp-phpunit adjustments

e78225b

More wp-phpunit adjustments

f9180b6

More wp-phpunit adjustments

ea1f0ab

Add test constants to override file

d93380b

remove debugging cruft

243aca9

Fix command name.

64e9280

TimothyBJacobs added 2 commits June 3, 2020 17:25

Fix php cs

4dbd0cf

Try removing crossenv ports for multisite.

3ea34ae

epiqueras approved these changes Jun 4, 2020

View reviewed changes

TimothyBJacobs merged commit 94f665e into master Jun 4, 2020

TimothyBJacobs deleted the fix/phpunit-multisite-and-redefined-const-errors branch June 4, 2020 00:53

github-actions bot added this to the Gutenberg 8.3 milestone Jun 4, 2020

oandregal changed the title ~~wp-env: correct multisite support~~ Env: correct multisite support Jun 8, 2020

noahtallen mentioned this pull request Jun 8, 2020

FSE Plugin: Add phpunit tests Automattic/wp-calypso#39328

Merged

ellatrix mentioned this pull request Jun 16, 2020

WP 5.5: merge all PHP changes #23210

Closed

12 tasks

This was referenced Jun 25, 2020

Use wp-env package for Docker environment GoogleForCreators/web-stories-wp#2521

Closed

Multisite support GoogleForCreators/web-stories-wp#2670

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Env: correct multisite support #22613

Env: correct multisite support #22613

noahtallen commented May 25, 2020 •

edited

Loading

github-actions bot commented May 25, 2020 •

edited

Loading

epiqueras commented May 25, 2020

noahtallen commented May 26, 2020

noahtallen commented May 26, 2020

noahtallen commented May 26, 2020

noisysocks commented May 26, 2020

noahtallen commented May 26, 2020

noahtallen commented May 26, 2020

noahtallen commented May 26, 2020

TimothyBJacobs commented May 26, 2020

noisysocks commented May 26, 2020

noisysocks commented May 26, 2020

noahtallen commented May 26, 2020

TimothyBJacobs commented May 26, 2020

noahtallen commented May 26, 2020

TimothyBJacobs commented May 26, 2020

epiqueras commented May 26, 2020

noahtallen commented May 26, 2020

noahtallen commented May 26, 2020

TimothyBJacobs commented May 26, 2020

noahtallen commented May 26, 2020

noahtallen commented May 26, 2020

TimothyBJacobs commented Jun 3, 2020

noahtallen commented Jun 3, 2020

epiqueras commented Jun 4, 2020

Env: correct multisite support #22613

Env: correct multisite support #22613

Conversation

noahtallen commented May 25, 2020 • edited Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

github-actions bot commented May 25, 2020 • edited Loading

epiqueras commented May 25, 2020

noahtallen commented May 26, 2020

noahtallen commented May 26, 2020

noahtallen commented May 26, 2020

noisysocks commented May 26, 2020

noahtallen commented May 26, 2020

noahtallen commented May 26, 2020

noahtallen commented May 26, 2020

TimothyBJacobs commented May 26, 2020

noisysocks commented May 26, 2020

noisysocks commented May 26, 2020

noahtallen commented May 26, 2020

TimothyBJacobs commented May 26, 2020

noahtallen commented May 26, 2020

TimothyBJacobs commented May 26, 2020

epiqueras commented May 26, 2020

noahtallen commented May 26, 2020

noahtallen commented May 26, 2020

TimothyBJacobs commented May 26, 2020

noahtallen commented May 26, 2020

noahtallen commented May 26, 2020

TimothyBJacobs commented Jun 3, 2020

noahtallen commented Jun 3, 2020

epiqueras commented Jun 4, 2020

noahtallen commented May 25, 2020 •

edited

Loading

github-actions bot commented May 25, 2020 •

edited

Loading