Improve theme.json test failure messages by pretty printing css for a more accurate diff #64077
Conversation
talldan
added
the
[Type] Automated Testing
|
Overall I love the idea of making these tests more readable and useful, by highlighting the actual CSS that has changed, I think that will help a lot in the developer experience of working with these 👍
In my experience with these tests, I think this could be a negative. It can be quite time consuming to update the tests, and ideally it would be possible to copy + paste the actual string in the error message to quickly update a test. Would it be possible to try including the original received raw string in the third param of the assertion (as the assertion message), to see if that can give folks a usable string to copy/paste to update the tests? From my perspective in updating these tests, we want to try to achieve two things at once, if we can:
|
Ok, I think I've found a way in ce6a6a6 to have both the pretty printed failures and the raw css string for updating the test case. It'll now output something like this: The only downside is that it outputs the line of code for the custom assertion at the bottom (85) as well as the actual line of code of the failure (755). Also the code might be considered sketchy/hacky, but I think it's ok. |
talldan
changed the title
|
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 If 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. |
talldan
added
the
No Core Sync Required
WIP Add comment
…re along with raw css for updating the test Update accidentally committed test failure Fix expected,actual order and minimize diff Fix - make sure the actual is output in addition to the pretty printed failure, not the expected Update test expectation so test passes Revert "Update test expectation so test passes" This reverts commit c9f655a. Update test expectation so test passes
talldan
force-pushed
the
try/theme-json-test-css-formatting
branch
from
97f668d to
ce6a6a6
Compare
| $this->assertSameCSS( $variables, $theme_json->get_stylesheet( array( 'variables' ) ) ); | ||
| $this->assertSameCSS( $styles, $theme_json->get_stylesheet( array( 'styles' ) ) ); | ||
| $this->assertSameCSS( $presets, $theme_json->get_stylesheet( array( 'presets' ) ) ); | ||
| $this->assertSameCSS( $all, $theme_json->get_stylesheet() ); |
There was a problem hiding this comment.
Very nice, I love how this abstraction means that we don't add any lines to individual tests 👍
There was a problem hiding this comment.
Great work @talldan! The pretty printing in test failures is going to make a huge difference to anyone working on updating these tests, and the new assertSameCSS method still allows for quick copy/pasting to update the affected tests. Also the inclusion of the error message is a win as it helps nudge folks toward how to fix a failing test when the changes are expected 👍
Looking good while testing locally running npm run test:unit:php:base -- --filter WP_Theme_JSON_Gutenberg_Test:
✅ Changes in CSS are now pretty printed correctly
✅ Copy + pasting the expected string into the test gets the test passing again
The only downside is that it outputs the line of code for the custom assertion at the bottom (85) as well as the actual line of code of the failure (755). Also the code might be considered sketchy/hacky, but I think it's ok.
The double output of lines here seems quite minor to me, and at least it flags where the error message is generated. Also, I see what you mean in terms of the code possibly being seen as hacky, but given the clear developer experience wins and that it'll help debug failing tests, I think it's worth it 👍
LGTM! 🚀
phpunit/class-wp-theme-json-test.php
Outdated
| // Pretty print CSS in test assertions so that it provides a better diff when a test fails. | ||
| // Without this: the failing test outputs the entire css string as being different. | ||
| // With this: the failing test only highlights the specific CSS rule that is different. |
There was a problem hiding this comment.
Tiny formatting nit: should this be a doc comment instead?
| * @param string $expected The expected CSS. | ||
| * @param string $actual The actual CSS. | ||
| */ | ||
| private function assertSameCSS( $expected, $actual, $message = '' ) { |
|
Thanks for the reviews, I'll merge this and then follow up with the same change in core. |
talldan
changed the title
What?
Related #60842
One of the issues with the theme json tests (the ones that people hate) is that when they fail they output the css diff as a big one-line string, and it's pretty much impossible to spot the difference. I had an idea to improve this, which is to apply some basic pretty printing to the CSS used in the assertion so that it's like a traditional stylesheet - on multiple lines.
When a test fails it'll now show a much more useful diff with only the specific selectors or rules that are actually different rather than all the css. I think this will make the tests much more useful, when making changes to production you'll now be able to see the exact impact.
This has been achieved using a custom assertion function that pretty prints the css strings.
The raw CSS is still useful for updating the failed test. To handle this, a custom message is appended on to the test failure containing the raw css. It does this by taking advantage of the fact that PHP unit test failures are thrown as exceptions. The exception from an assertion can be caught and re-thrown but with some extra information appended.
Testing Instructions
Screenshots or screencast
Before
After