First pass at refactoring theme json class tests #6734
Conversation
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
| * @ticket 61165 | ||
| */ | ||
| public function test_get_stylesheet_support_for_shorthand_and_longhand_values() { | ||
| public function test_get_styles_for_block_support_for_shorthand_and_longhand_values() { |
There was a problem hiding this comment.
The logic for outputting styles for longhand and shorthand values is in get_styles_for_block, so better to test that function directly.
| $this->assertSame( $all, $theme_json->get_stylesheet() ); | ||
| $this->assertSame( $styles, $theme_json->get_stylesheet( array( 'styles' ) ) ); | ||
| $this->assertSame( $all, $theme_json->get_stylesheet( array( 'styles', 'presets', 'variables' ), null, array( 'skip_root_layout_styles' => true ) ) ); | ||
| $this->assertSame( $styles, $theme_json->get_stylesheet( array( 'styles' ), null, array( 'skip_root_layout_styles' => true ) ) ); |
There was a problem hiding this comment.
By skipping root layout styles, we can remove a good chunk of output which is irrelevant to this test and makes reading the test strings harder. I've done this for a few tests, whereever it makes sense to test the output from get_stylesheet but we don't care about the whole output.
| * @ticket 60936 | ||
| * @ticket 61165 | ||
| */ | ||
| public function test_get_stylesheet_preset_values_are_marked_as_important() { |
There was a problem hiding this comment.
This was essentially a duplicate of the other preset tests - if the CSS changes in any way, those tests will flag it so it's redundant to have this.
| * @ticket 61165 | ||
| */ | ||
| public function test_get_stylesheet_handles_whitelisted_element_pseudo_selectors() { | ||
| public function test_get_styles_for_block_handles_whitelisted_element_pseudo_selectors() { |
There was a problem hiding this comment.
I'm not sure about this one because the whitelisting actually happens in get_stylesheet. I think it's good to have a test specifically for get_styles_for_block because there's already a bunch testing pseudo selectors with get_stylesheet. Perhaps we can keep this but change its name.
| * @ticket 60936 | ||
| * @ticket 61165 | ||
| */ | ||
| public function test_get_stylesheet_handles_whitelisted_block_level_element_pseudo_selectors() { |
There was a problem hiding this comment.
This is essentially a duplicate of the test above.
|
|
||
| $expected = ':root { --wp--style--global--content-size: 800px;--wp--style--global--wide-size: 1000px; }:where(body) { margin: 0; }.wp-site-blocks > .alignleft { float: left; margin-right: 2em; }.wp-site-blocks > .alignright { float: right; margin-left: 2em; }.wp-site-blocks > .aligncenter { justify-content: center; margin-left: auto; margin-right: auto; }:where(.is-layout-flex){gap: 0.5em;}:where(.is-layout-grid){gap: 0.5em;}.is-layout-flow > .alignleft{float: left;margin-inline-start: 0;margin-inline-end: 2em;}.is-layout-flow > .alignright{float: right;margin-inline-start: 2em;margin-inline-end: 0;}.is-layout-flow > .aligncenter{margin-left: auto !important;margin-right: auto !important;}.is-layout-constrained > .alignleft{float: left;margin-inline-start: 0;margin-inline-end: 2em;}.is-layout-constrained > .alignright{float: right;margin-inline-start: 2em;margin-inline-end: 0;}.is-layout-constrained > .aligncenter{margin-left: auto !important;margin-right: auto !important;}.is-layout-constrained > :where(:not(.alignleft):not(.alignright):not(.alignfull)){max-width: var(--wp--style--global--content-size);margin-left: auto !important;margin-right: auto !important;}.is-layout-constrained > .alignwide{max-width: var(--wp--style--global--wide-size);}body .is-layout-flex{display: flex;}.is-layout-flex{flex-wrap: wrap;align-items: center;}.is-layout-flex > :is(*, div){margin: 0;}body .is-layout-grid{display: grid;}.is-layout-grid > :is(*, div){margin: 0;}'; | ||
| $root_rules = $theme_json->get_root_layout_rules( WP_Theme_JSON::ROOT_BLOCK_SELECTOR, $metadata ); | ||
| $style_rules = $theme_json->get_styles_for_block( $metadata ); |
There was a problem hiding this comment.
get_styles_for_block wasn't actually outputting anything here, so I removed it.
|
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 Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
andrewserong
left a comment
andrewserong
left a comment
There was a problem hiding this comment.
This is looking terrific to me:
✅ The readability of the updated tests is greatly improved
✅ Each instance of a reduction in scope in the tests better highlights what the test was aiming to cover
✅ The test removals and comments as to why things were changed appear logical, and overall this doesn't appear to result in a meaningful reduction of test coverage
This looks great to me. It'd be worth also getting a second opinion, though, but overall I think this will help improve both the readability and maintainability of these tests, especially when making changes to any of the styles they cover.
Nice work!
|
LGTM I like how this change brings more specificity to coverage, e.g., testing the method that outputs block CSS. I think it'll make life easier when debugging. And also thanks for deleting a bunch of those CSS expectations 🙃 |
|
Committed in r58378. |
3 participants
Trac ticket: https://core.trac.wordpress.org/ticket/61371
Update: this is now ready for review.
These changes should increase the stability of these tests and make maintaining them less painful by reducing test strings to only the styles that are relevant to each test.
I used 3 types of approach:
refactoring tests that call
use_stylesheetwhen they really want to test the behaviour of one of its underlying functions to instead call that underlying function, using a reflection class where needed (the underlying function is usuallyget_styles_for_block)skipping output of base layout styles with
'skip_root_layout_styles' => truewhere the test is checking multiple parts ofuse_stylesheetoutput but base layout styles aren't relevant;deleting tests that are already covered by other tests.
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.