-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Backport theme.json tests from Core #58476
Conversation
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/class-wp-theme-json-schema-gutenberg.php ❔ phpunit/class-wp-theme-json-schema-test.php ❔ phpunit/data/themedir1/block-theme-child/styles/variation-b.json ❔ phpunit/data/themedir1/block-theme/styles/variation.json ❔ lib/class-wp-theme-json-gutenberg.php ❔ lib/class-wp-theme-json-resolver-gutenberg.php ❔ lib/load.php ❔ phpunit/class-wp-rest-global-styles-controller-gutenberg-test.php ❔ phpunit/class-wp-theme-json-resolver-test.php ❔ phpunit/class-wp-theme-json-test.php ❔ phpunit/data/languages/themes/block-theme-pl_PL.mo ❔ phpunit/data/languages/themes/block-theme-pl_PL.po ❔ phpunit/data/themedir1/block-theme/theme.json |
Flaky tests detected in a1c948a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7717738234
|
e6169d5
to
6a181f4
Compare
cccaf59
to
5ee08b4
Compare
@@ -2638,7 +2638,7 @@ public function get_root_layout_rules( $selector, $block_metadata ) { | |||
$css .= '--wp--style--global--wide-size: ' . $wide_size . ';'; | |||
} | |||
|
|||
$css .= '}'; |
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.
There were a bunch of places in the tests where this was different from core, so I figured I'd just add the space to make them the same. This is the only non-test change.
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.
The tests are all passing for me, and nothing stands out as odd when I look through all the diffs. Admittedly, there are a lot so it's hard to comb through! Thanks for being so tedious about this!
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @jeryj. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. Core SVNIf you're a Core Committer, use this list when committing to
GitHub Merge commitsIf you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
@@ -4,6 +4,8 @@ | |||
* Test WP_Theme_JSON_Resolver_Gutenberg class. | |||
* | |||
* @package Gutenberg | |||
* | |||
* @since 5.8.0 | |||
*/ | |||
|
|||
class WP_Theme_JSON_Resolver_Gutenberg_Test extends WP_UnitTestCase { |
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.
In core this is called Tests_Theme_wpThemeJsonResolver
. Should we rename this to Tests_Theme_wpThemeJsonResolver_Gutenberg
?
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.
I wonder if we should match the file path too -> tests/phpunit/tests/theme/wpThemeJsonResolver.php
@@ -695,7 +695,7 @@ public function __construct( $theme_json = array(), $origin = 'theme' ) { | |||
$origin = 'theme'; | |||
} | |||
|
|||
$this->theme_json = WP_Theme_JSON_Schema::migrate( $theme_json ); | |||
$this->theme_json = WP_Theme_JSON_Schema_Gutenberg::migrate( $theme_json ); |
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.
It would be great if this renaming was part of the build process....
* | ||
* @since 5.9.0 | ||
*/ | ||
class WP_Theme_JSON_Schema_Gutenberg_Test extends WP_UnitTestCase { |
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.
Similar comments as above for this file
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.
I think this looks good. I left a few comments but nothing that should stop us from merging this.
Personally, I like the Gutenberg naming convention for the class/file name. It matches the class/file it's testing so the connection makes more sense to me. If Core has a good reason for the conventions it uses, then go with that. But I couldn't source the decisions for why it was renamed when merged with Core.
Absolutely. There's a few things that I think need to happen first to make it automatable.
I would love to see an automatic PR opened in Gutenberg when Core changes are made and vice versa, or at the very least a patch generated that can easily be turned into a PR. |
What?
Merges changes with Core for the following tests:
Paired with WordPress/wordpress-develop#5979.
Why?
Backport.
How?
I tried to be as methodical as possible with the changes.
_Gutenberg
postfixes are applied/removed.ticket
comments.Partial diffs
Here are some partial diffs between Gutenberg and Core that don't include comments or renames.
WP_REST_Global_Styles_Controller_Gutenberg_Test
WP_Theme_JSON_Resolver_Gutenberg_Test
WP_Theme_JSON_Schema_Gutenberg_Test
WP_Theme_JSON_Gutenberg_Test
There are a few things of note:
emptytheme
in Gutenberg andtt1-blocks
in core. I found a comment that seemed to indicate that was intentional, so I left it that way. However, it means the diff isn't as clean.Full diffs
Here are the full diffs between Gutenberg and Core for reference.
WP_REST_Global_Styles_Controller_Gutenberg_Test
WP_Theme_JSON_Resolver_Gutenberg_Test
WP_Theme_JSON_Schema_Gutenberg_Test
WP_Theme_JSON_Gutenberg_Test
Testing Instructions
Run the PHPUnit tests for the following filters:
Testing Instructions for Keyboard
N/A
Screenshots or screencast
N/A