-
Notifications
You must be signed in to change notification settings - Fork 805
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dev: Only one WordPress #41057
base: trunk
Are you sure you want to change the base?
Dev: Only one WordPress #41057
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Saw this in a list and the title piqued my curiosity 😀
I wonder if the packages/test-environment
package is really needed. It seems to do about the same thing as just sticking something like this in each consumer's bootstrap.php would.
require dirname( __DIR__, 5 ) . '/tools/php-test-env/vendor/autoload.php';
\WorDBless\Load::load();
(vary the 5
as needed based on the directory structure). The one added require
seems less boilerplate than a whole extra package that in the end does the same thing.
If you still want the additional error handling, make a tools/php-test-env/load.php
file to require instead. 🤷
9609fb1
to
81ed25b
Compare
648c1e0
to
f250081
Compare
b38d9dc
to
16fdf6f
Compare
# Add WordBless for image-cdn package tests | ||
if [[ "$TEST_SCRIPT" =~ ^test-(php|coverage)$ ]] && jq -e '.name == "automattic/jetpack-image-cdn"' projects/packages/image-cdn/composer.json >/dev/null; then | ||
echo "Adding WordBless for image-cdn tests" | ||
composer require --working-dir=projects/packages/image-cdn automattic/wordbless:^0.4.2 --dev | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this here rather than in projects/packages/image-cdn/composer.json
? If someone tries to run image-cdn tests locally, won't it blow up because of missing this?
Ah, I see a comment later mentions concurrent tests, and digging a little more finds that WorDBless has an @after
method that clears the uploads dir. I also see a packages/publicize test failed for what looks the same reason in a recent run, and packages/videopress in another.
Probably the thing to do would be to update WorDBless to use wp_upload_dir()
to find the upload dir instead of assuming the default, and then have monorepo tests override the default per package so they don't stomp on each other.
@@ -27,6 +27,7 @@ | |||
// Ignore stuff in various subdirs too. | |||
'.*/node_modules/', | |||
'tools/docker/', | |||
'tools/php-test-env/wordpress/', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides this, you'll also need to exclude the tools/php-test-env/vendor
from being analyzed by adding something like this into the $options
array here.
'exclude_analysis_directory_list' => array(
'tools/php-test-env/vendor',
),
(can't make it a suggestion because it has to go outside of where GH will allow comments, e.g. just after line 36 below).
For unit testing, we often use WorDBless, a variant of WordPress that does not require database setup. Except, we include it a lot so there are a lot of copies of WordPress within the repo when fully installed.
Before and after the changes proposed below (conducted in clean directories on my M1 with composer's cache cleared)
Before:
1st run of
jetpack install --all
: Between 2m 57s and 3m 17s2nd run: 56s
Size: (du of monorepo with du of .git subtracted): 5.1 GB
After:
1st run: Between 1m 20s and 1m 52s
2nd run: 55s
Size: 2.7 GB
Proposed changes:
test-environment
package to serve as the WorDBless loader for monorepo projects.jetpack install --all, -r
will cover it.In short, this changes the packages consumed by individual projects to use the internal test-environment. This ensure it is easy to modify all in the future and required little change to each individual package.
The test-environment loads a class that loads the test-php-env autoloader and loads WorDBless.
The test-php-env quasi-package does a singular install of WorDBless.
Other solution investigated was having the
test-environment
package directly require WorDBless, but that meant it would be re-installed every timetest-environment
was installed. No gain (at least) to install time, but it would have ended up with only one copy of WP.Other information:
Jetpack product discussion
None yet.
Does this pull request change what data or activity we track or use?
No.
Testing instructions:
jetpack install -r
(at least) andjetpack install
a package that uses worDBLess, likepackages/admin-ui
jetpack test phh packages/admin-ui
Does it still run the test successfully?