Skip to content
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

Workflows: Run PHP unit tests also against current WP - 1 #46983

Merged
merged 7 commits into from
Apr 5, 2023

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Jan 9, 2023

What?

Run PHP unit tests also against the previous version of WordPress.

Fixes #46979.

Why?

tl;dr: Because Gutenberg is supposed to be compatible with the latest WP version and with the one before it, but sometimes breaks when installed on the latter

See #46979 for more background.

How?

Extends the PHP unit test matrix so that when run with PHP 7.4, tests are not only run for the current WordPress version, but also for the one before.

Testing Instructions

See CI.

@ockham ockham added the [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. label Jan 9, 2023
@ockham ockham self-assigned this Jan 9, 2023
@ockham ockham force-pushed the add/unit-test-against-current-wp-minus-one branch 2 times, most recently from 581ed06 to 3afff6a Compare January 9, 2023 14:18
include:
# Test with the previous WP version.
- php: '7.4'
wordpress: '6.0'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a follow-up, we could consider computing the previous WP series automatically (i.e. figure out that 6.1 is the current series, and that thus, 6.0 is the previous one).


env:
WP_ENV_PHP_VERSION: ${{ matrix.php }}
WP_ENV_CORE: WordPress/WordPress${{ matrix.wordpress != '' && format( '#{0}-branch', matrix.wordpress ) || '' }}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The advantage of using the WordPress/WordPress git repo's x.y-branch is that we don't have to compute the latest .patch version in that series (would be 6.0.3 here).

@ockham ockham marked this pull request as ready for review January 9, 2023 14:36
@github-actions
Copy link

github-actions bot commented Jan 9, 2023

Flaky tests detected in a0e62f3.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4607133381
📝 Reported issues:

@ockham
Copy link
Contributor Author

ockham commented Jan 10, 2023

The newly added WP 6.0 test is currently failing with

> gutenberg@14.9.0-rc.1 test:unit:php /home/runner/work/gutenberg/gutenberg
> wp-env run tests-wordpress /var/www/html/wp-content/plugins/gutenberg/vendor/bin/phpunit -c /var/www/html/wp-content/plugins/gutenberg/phpunit.xml.dist --verbose

- 
ℹ Starting '/var/www/html/wp-content/plugins/gutenberg/vendor/bin/phpunit -c /var/www/html/wp-content/plugins/gutenberg/phpunit.xml.dist --verbose' on the tests-wordpress container. 

Creating 34897e1850d618b89954e510f52aa54b_tests-wordpress_run ... 
Creating 34897e1850d618b89954e510f52aa54b_tests-wordpress_run ... done
[09-Jan-2023 14:41:09 UTC] PHP Warning:  require_once(/var/www/html/wp-includes/class-wpdb.php): failed to open stream: No such file or directory in /wordpress-phpunit/includes/install.php on line 43
[09-Jan-2023 14:41:09 UTC] PHP Fatal error:  require_once(): Failed opening required '/var/www/html/wp-includes/class-wpdb.php' (include_path='.:/usr/local/lib/php') in /wordpress-phpunit/includes/install.php on line 43

Does that ring a bell for anyone? Some kind of wordpress-phpunit version incompatibility? Some missing env var or argument for wp-env? 🤔

@anton-vlasenko anton-vlasenko force-pushed the fix/parity-with-cores-php-ci-jobs branch 2 times, most recently from 2ce991c to 2a371d8 Compare January 16, 2023 10:27
Base automatically changed from fix/parity-with-cores-php-ci-jobs to trunk January 16, 2023 12:49
@ockham ockham force-pushed the add/unit-test-against-current-wp-minus-one branch from e2d7611 to 945ca4e Compare January 16, 2023 13:56
@ockham
Copy link
Contributor Author

ockham commented Jan 16, 2023

The newly added WP 6.0 test is currently failing with

> gutenberg@14.9.0-rc.1 test:unit:php /home/runner/work/gutenberg/gutenberg
> wp-env run tests-wordpress /var/www/html/wp-content/plugins/gutenberg/vendor/bin/phpunit -c /var/www/html/wp-content/plugins/gutenberg/phpunit.xml.dist --verbose

- 
ℹ Starting '/var/www/html/wp-content/plugins/gutenberg/vendor/bin/phpunit -c /var/www/html/wp-content/plugins/gutenberg/phpunit.xml.dist --verbose' on the tests-wordpress container. 

Creating 34897e1850d618b89954e510f52aa54b_tests-wordpress_run ... 
Creating 34897e1850d618b89954e510f52aa54b_tests-wordpress_run ... done
[09-Jan-2023 14:41:09 UTC] PHP Warning:  require_once(/var/www/html/wp-includes/class-wpdb.php): failed to open stream: No such file or directory in /wordpress-phpunit/includes/install.php on line 43
[09-Jan-2023 14:41:09 UTC] PHP Fatal error:  require_once(): Failed opening required '/var/www/html/wp-includes/class-wpdb.php' (include_path='.:/usr/local/lib/php') in /wordpress-phpunit/includes/install.php on line 43

Does that ring a bell for anyone? Some kind of wordpress-phpunit version incompatibility? Some missing env var or argument for wp-env? 🤔

Looks like that the file that wordpress-phpunit is looking for (wp-includes/class-wpdb.php) was indeed named differently (/wp-includes/wp-db.php) in WP 6.0 (see).

So we might want a mechanism to also fetch a different version of wordpress-phpunit. I vaguely seem to recall that we had something like that in wp-env, but I'm not 100% sure. @noahtallen to the rescue? 🙏 😊

@ockham
Copy link
Contributor Author

ockham commented Jan 16, 2023

Ah, IIUC, per #41780, wp-env should be downloading the correct version of wordpress-phpunit. Looks like something isn't quite working then... 🤔

@noahtallen
Copy link
Member

noahtallen commented Jan 17, 2023

wp-env should be downloading the correct version of wordpress-phpunit

Right on... I do remember this issue, but not sure where I saw it before. Probably in that PR! I guess it'd be worth adding loggind and debug information related to wp-env.

Idea: wp-env could be caching something and not updating. It attempts to run from cache to be fast. However, the cache buster should take into account at a minimum .wp-env.json changes.

We could verify the logic to ensure that it correctly updates if the global variable changes.

We could also run wp-env start --update instead of wp-env start. This would force it to download WordPress and the testing libraries again.

Copy link
Contributor

@anton-vlasenko anton-vlasenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for submitting the PR!
Please find my questions below.

.github/workflows/unit-test.yml Show resolved Hide resolved
include:
# Test with the previous WP version.
- php: '7.4'
wordpress: '6.0'
Copy link
Contributor

@anton-vlasenko anton-vlasenko Feb 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that mean this string need to be changed when a new WordPress comes out and the previous version gets outdated? If so, is there a way to automate it (so no manual changes are needed)? I'm asking just out of curiosity.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I would like to automate this (and actually even had written a comment on it that was probably somewhat lost in rebases). I didn't want to make it a blocker for this PR though as it might get a bit involved.

(We'd likely get the latest version from this WordPress.org REST API endpoint, truncate the point version to get the "series", and then infer the previous series from it. E.g. 6.0.3 -> 6.0 -> 5.9. Requires a bit of version arithmetics that I thought would be a nice fit for a separate PR. We have some similar logic e.g. here.)

Copy link
Contributor

@anton-vlasenko anton-vlasenko Feb 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s just a thought, but what if wpenv supported something like

"core": "WordPress/WordPress#previous-stable",

or just

"core": "WordPress/WordPress#previous",

?
This way we wouldn't have to write any custom scripts and it will work everywhere (not just in GitHub CI workflows).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a good idea, but we'd probably need different syntax. That basically just converts directly to git -- e.g. github.com/WordPress/Wordpress, with # corresponding to a git ref (like a tag)

@ockham ockham force-pushed the add/unit-test-against-current-wp-minus-one branch from 945ca4e to 063f5d0 Compare February 20, 2023 13:19
@anton-vlasenko
Copy link
Contributor

anton-vlasenko commented Feb 20, 2023

I will do my best to test this PR today/tomorrow. Thanks.

@ockham
Copy link
Contributor Author

ockham commented Feb 21, 2023

We could also run wp-env start --update instead of wp-env start. This would force it to download WordPress and the testing libraries again.

@noahtallen I just tried this in 063f5d0, but unfortunately, tests are still failing with the same error.

Copy link
Contributor

@anton-vlasenko anton-vlasenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please find my comment below. Thanks.


env:
WP_ENV_PHP_VERSION: ${{ matrix.php }}
WP_ENV_CORE: WordPress/WordPress${{ matrix.wordpress != '' && format( '#{0}-branch', matrix.wordpress ) || '' }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested this PR, and I don't think -branch needs to be appended to the name of the WP version.
When I log in to the docker container, I get 6.0.4-alpha-55372 as the WP version in src/wp-includes/version.php, but it should be 6.0 instead.

Copy link
Contributor Author

@ockham ockham Feb 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I agree 😅 In the example you're giving, I'd want us to use 6.0.3. That's currently the latest version from the 6.0 series that has all the bugfixes that were deemed critical enough to warrant a point release (well, three of them); and I believe that the auto-updater would update most people's WordPress installation to 6.0.3, if they had previously installed 6.0 (or 6.0.1 or 6.0.2).

Using -branch is a cheap way of getting that latest version (that doesn't require us to do any REST API lookups). Now since we're fetching our WordPress from git, we're not getting 6.0.3 exactly, but the latest state of the 6.0 branch, which might have a few commits on top; and which has a weird 6.0.4-alpha-something version tag, as you've found.

I still find that preferable over using 6.0. Why? Imagine there is a bug in 6.0 that causes a problem (manifested by a unit test error) if the Gutenberg plugin is active. We can fix that bug, and it could make its way into a 6.0.x point release, but it'd never get into 6.0! This means the tests just would keep failing.

The optimal solution would likely be to actually compute the latest version from the 6.0 series (via something like this). Personally, I'd be fine keeping the -branch approach for a first iteration, and do a separate PR to do the version number computation later. Would that be okay?

Copy link
Contributor

@anton-vlasenko anton-vlasenko Feb 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point, and I agree it's better to use the latest minor release (if available).
But, I think using 6.0-branch is less optimal than 6.0.3 because regular WordPress users don't usually update their websites to the latest "branch" version.
They update to the latest minor or major release using the WordPress dashboard.
From my point of view, it makes more sense to test Gutenberg with WP versions that most users can use and update to.

Personally, I'd be fine keeping the -branch approach for a first iteration, and do a separate PR to do the version number computation later. Would that be okay?

Yes, I think that is fine. Thanks.

Copy link
Contributor Author

@ockham ockham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just re-iterating that this PR isn't currently working -- we still need to fix this issue 😬

@anton-vlasenko
Copy link
Contributor

anton-vlasenko commented Feb 23, 2023

@ockham

Just re-iterating that this PR isn't currently working -- we still need to fix this issue 😬

I think I know why it's happening.
It's because this code in plugins/gutenberg/packages/env/lib/download-wp-phpunit.js always downloads the trunk version no matter what:

if ( ! wpVersion || wpVersion.match( /-(?:alpha|beta|rc)/i ) ) {
		ref = 'trunk';
		fetchRaw.push( 'fetch', 'origin', ref, '--depth', '1' );
	} else {
		ref = `tags/${ wpVersion }`;
		fetchRaw.push( 'fetch', 'origin', 'tag', wpVersion, '--depth', '1' );
	}
}

Debug output:

wpVersion : 6.0.4-alpha-55372
ref : trunk
fetchRaw : [ 'fetch', 'origin', 'trunk', '--depth', '1' ]

So it's fetching trunk despite the value of the wpVersion.

@anton-vlasenko
Copy link
Contributor

anton-vlasenko commented Mar 7, 2023

I've investigated why these test fail, @ockham.
The error in WP_Theme_JSON_Resolver_Gutenberg_Test::test_get_user_data_from_wp_global_styles_does_not_use_uncached_queries occurs because the Core version of WP_Theme_JSON_Resolver does not rely on the low-level WP_Query cache to fetch user styles (PR: #45634). However, the Gutenberg version of the same class, WP_Theme_JSON_Resolver_Gutenberg, does use it. This error seems to be caused by a mismatch between the unit test code in Gutenberg and the old code in Core.

The same applies to the error in WP_Theme_JSON_Gutenberg_Test::test_allow_indirect_properties. It occurs because row-gap has been added as an allowed attribute in the safecss_filter_attr function.

P.S. I've just asked myself what we are really doing here.
That new CI job runs old WordPress code and the current Gutenberg PHPUnit tests.
So, it kind of "assumes" that the WordPress code will work the same.
However, I have found that this is not always the case. In fact, these failed tests have not helped to reveal any bugs, but have merely shown that the code has changed since the last WordPress release.
Therefore, I have doubts about the practical usefulness of this new CI job unless we have a reliable mechanism to skip "incompatible" tests.
What is your opinion on this matter?

@ockham
Copy link
Contributor Author

ockham commented Mar 9, 2023

Thank you very much for investigating @anton-vlasenko !

That new CI job runs old WordPress code and the current Gutenberg PHPUnit tests.
So, it kind of "assumes" that the WordPress code will work the same.

Hmm 🤔 My mental model was a bit different here: We're running old WordPress code plus current Gutenberg code, which includes the "compatibility layer" code that includes versions of classes (and functions) such as WP_Theme_JSON_Resolver_Gutenberg which have updates compared to the underlying Core version (WP_Theme_JSON_Resolver). And it seemed to make sense to me to have unit tests for the Gutenberg versions of those classes. If we'd see errors when running those tests against the previous WordPress version, they'd hint at a problem with how the GB compatibility class is implemented -- after all, we are running the unit test to check the behavior of WP_Theme_JSON_Resolver_Gutenberg (and not WP_Theme_JSON_Resolver), are we? 🤔

cc/ @oandregal since the code (and tests) in question seems to fall into your area of expertise 😊

@oandregal
Copy link
Member

oandregal commented Mar 15, 2023

👋 I see the WP_Theme_JSON_Gutenberg_Test::test_allow_indirect_properties test has been introduced #46388 so I'd ping @andrewserong @tellthemachines and @mmtr as they know a lot more than I do.

(I'm looking at the other failing test. I'm in the middle of a work trip, so I don't know how far I'll be able to dig into this this week).

@oandregal
Copy link
Member

@ockham I've taken a look at the test. #45634 was the PR that introduced it. It backported WordPress/wordpress-develop#3517 that was the one that introduced the change.

As far as I could understand, the change there was to remove the existing caching in the resolver and make dependand on another cache that exists in wp_query. I don't know when this wp_query cache was introduced. If it was introduced in 6.1 but it's not present in 6.0, it'd explain the difference. I'm pinging the original authors of the PR in case they can provide any advice @peterwilsoncc @hellofromtonya @andrewserong

In any case, it sounds like test_get_user_data_from_wp_global_styles_does_not_use_uncached_queries is not testing the actual WP_Theme_JSON_Resolver or WP_Theme_JSON_Resolver_Gutenberg classes, but something else that it uses and lives in core directly. I'd argue that it can be removed. Another thought is: in a few days/weeks, we'll make 6.1 the minimum WordPress version, which I understand also solves the issue for this particular test.

@andrewserong
Copy link
Contributor

WP_Theme_JSON_Gutenberg_Test::test_allow_indirect_properties

Thanks for the ping! This is a really interesting one, and I think this PR has actually revealed something we missed during the backporting process of some of the layout changes for 6.1.

The TL;DR is that for Gutenberg to be properly compatible with WordPress 6.0, we missed adding in a couple of required CSS properties (row-gap and column-gap) in the list of safe_style_css properties. I have an open PR in #49118 that adds in these missing properties — feel free to copy+paste the changes from there to this PR if it's easier.

More background:

So, from the sounds of it, for this test #49118 should fix it up. And the good thing is that it's revealed the value of adding in a job to tests against the previous WP version! I don't think we would have caught this issue otherwise.

@andrewserong
Copy link
Contributor

Update: I've merged in #49118, so after a rebase, this PR should hopefully pass the WP_Theme_JSON_Gutenberg_Test::test_allow_indirect_properties test.

@ockham
Copy link
Contributor Author

ockham commented Mar 22, 2023

Thank you so much @oandregal and @andrewserong! It'd be amazing if this PR already helped unearth a compatibility issue with WP 6.0 😄

Gonna rebase now; crossing my fingers that the test will pass afterwards 🤞

@ockham ockham force-pushed the add/unit-test-against-current-wp-minus-one branch from 8bd8f52 to 1b95e3c Compare March 22, 2023 17:09
@ockham
Copy link
Contributor Author

ockham commented Mar 22, 2023

Looks like the WP_Theme_JSON_Gutenberg_Test::test_allow_indirect_properties test is passing now indeed 🥳

The test_get_user_data_from_wp_global_styles_does_not_use_uncached_queries failure still persists, so we might look into that one a bit more:

There was 1 failure:

1) WP_Theme_JSON_Resolver_Gutenberg_Test::test_get_user_data_from_wp_global_styles_does_not_use_uncached_queries
Unexpected SQL queries detected for the wp_global_style post type prior to creation.
Failed asserting that 3 is identical to 0.

/var/www/html/wp-content/plugins/gutenberg/phpunit/class-wp-theme-json-resolver-test.php:340
phpvfscomposer:///var/www/html/wp-content/plugins/gutenberg/vendor/phpunit/phpunit/phpunit:97

@ockham
Copy link
Contributor Author

ockham commented Mar 22, 2023

The test_get_user_data_from_wp_global_styles_does_not_use_uncached_queries failure still persists, so we might look into that one a bit more

Highlighting this suggestion from @oandregal :

In any case, it sounds like test_get_user_data_from_wp_global_styles_does_not_use_uncached_queries is not testing the actual WP_Theme_JSON_Resolver or WP_Theme_JSON_Resolver_Gutenberg classes, but something else that it uses and lives in core directly. I'd argue that it can be removed. Another thought is: in a few days/weeks, we'll make 6.1 the minimum WordPress version, which I understand also solves the issue for this particular test.

@ockham
Copy link
Contributor Author

ockham commented Mar 28, 2023

Since WP 6.2 is scheduled to be released tomorrow, let's wait a bit longer and then rebase this PR to see what happens once WP-1 becomes 6.1. Maybe that will resolve our remaining open issue -- hopefully it will not introduce a new one 🤞

@ockham ockham force-pushed the add/unit-test-against-current-wp-minus-one branch from 1b95e3c to 1a22fbe Compare March 30, 2023 17:22
@ockham
Copy link
Contributor Author

ockham commented Mar 30, 2023

image

(link)

🎉 🎉 🎉

Anyone wanna give approval to merge this? 😊

@ockham ockham force-pushed the add/unit-test-against-current-wp-minus-one branch from e63b159 to 3dd4a46 Compare April 4, 2023 11:05
Copy link
Member

@noahtallen noahtallen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Seems to be working well. One concern is just how many of these get added to each PR. Not sure how GitHub actions allocates boxes to CI builds, but could see us reaching some kind of resource limit. At some point, it might make sense to try to optimize the speed of each build

@ockham
Copy link
Contributor Author

ockham commented Apr 5, 2023

Nice! Seems to be working well.

Thank you, Noah!

One concern is just how many of these get added to each PR. Not sure how GitHub actions allocates boxes to CI builds, but could see us reaching some kind of resource limit. At some point, it might make sense to try to optimize the speed of each build

Yeah, indeed. I'm slightly concerned about the added jobs as well, but I guess let's see how it pans out and if we can optimize things a bit 😊

@ockham ockham merged commit e1eb90b into trunk Apr 5, 2023
@ockham ockham deleted the add/unit-test-against-current-wp-minus-one branch April 5, 2023 15:32
@github-actions github-actions bot added this to the Gutenberg 15.6 milestone Apr 5, 2023
test-php:
name: PHP ${{ matrix.php }}${{ matrix.multisite && ' multisite' || '' }} on ubuntu-latest
name: PHP ${{ matrix.php }}${{ matrix.multisite && ' multisite' || '' }}${{ matrix.wordpress != '' && format( ' (WP {0}) ', matrix.wordpress ) || '' }} on ubuntu-latest
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A problem with having the WordPress version in the name is that each time the version changes, someone has to update the required jobs in the repo settings. This slack thread discusses a situation where it happened today:
https://wordpress.slack.com/archives/C02QB2JS7/p1719295939361199

It'd be good to think about possible solutions to this. Only including the major version number (e.g. 6.4.x) might be one option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Workflows: Run PHP unit tests against previous WP version
7 participants