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

Improvements to class WP_Theme_JSON and its tests #60842

Open
tellthemachines opened this issue Apr 18, 2024 · 2 comments
Open

Improvements to class WP_Theme_JSON and its tests #60842

tellthemachines opened this issue Apr 18, 2024 · 2 comments
Assignees
Labels
CSS Styling Related to editor and front end styles, CSS-specific issues. [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Type] Enhancement A suggestion for improvement.

Comments

@tellthemachines
Copy link
Contributor

What problem does this address?

Currently, any change made to styles generated by class WP_Theme_JSON cause multiple tests from its test suite to fail, mostly due to diffs in CSS strings being checked for equality.

Adjustments to CSS output, whether properties or rule specificity, are very common, and test strings are incredibly painful to update when they inevitably fail.

The biggest culprits are the 30 or so tests that check the output of get_stylesheet. I think the main reason there are so many tests focusing on that function is that it brings together the output of a bunch of protected functions such as get_layout_styles, get_block_classes and get_css_variables, among others, and it's being used essentially to test the output of those functions.

What is your proposed solution?

How can this be improved?

I don't love the idea of tests comparing CSS strings, because the outcome of the test relies too much on implementation details (the exact shape of the CSS string, which the get_stylesheet function is not even responsible for). A possible alternative would be to check for the presence of one or another property or classname inside the string, instead of trying to match the whole string.

In general, unit tests are useful to check that the behaviour of a function remains unchanged after an edit or refactor. If we're testing an exact string of CSS, then all the changes we make to these functions will alter the output, because our styles are continually evolving. So what's the point of these unit tests? They're never catching any actual bugs. If we can abstract what we're checking for a bit - say we check that the function outputs a valid CSS string, but not its exact shape - maybe the tests become more useful while not being such a maintenance burden.

I'd love to hear about other potential approaches here!

Other things we can do to mitigate the impact of these tests on developer experience:

  • Test output of functions called by get_stylesheet independently.
  • Use a minimal set of properties for each test, so the test strings are smaller and easier to update when they inevitably change.

Further improvements for class WP_Theme_JSON itself:

  • Look into removing the custom CSS-specific functions in favour of using the general global styles ones, as per this comment.
  • There's a get_svg_filters function that isn't being used anywhere in either core or gutenberg, should we deprecate it? A quick check in https://www.wpdirectory.net/ doesn't show any use either.
@tellthemachines tellthemachines added [Type] Enhancement A suggestion for improvement. [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. CSS Styling Related to editor and front end styles, CSS-specific issues. labels Apr 18, 2024
@ajlende
Copy link
Contributor

ajlende commented Apr 19, 2024

There's a get_svg_filters function that isn't being used anywhere in either core or gutenberg, should we deprecate it?

This has been unused since #49103. Seems like we remembered to deprecate get_global_styles_svg_filters that called it when the global_styles_render_svg_filters hook was removed, but forgot to deprecate get_svg_filters.

@talldan
Copy link
Contributor

talldan commented Jul 30, 2024

I've made a draft PR with an idea for how we can improve the output of those tests - #64077

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CSS Styling Related to editor and front end styles, CSS-specific issues. [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

No branches or pull requests

3 participants