Revert "Revert test data for WithSlug variation (#62579)"

[oandregal]

Reverts #62579
Follow-up to #62550

This PR brings back one test we had to put in quarantine to unblock other PRs from lading.

[github-actions]

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

❔ phpunit/data/themedir1/block-theme/styles/block-style-variation-with-slug.json
❔ phpunit/block-supports/block-style-variations-test.php
❔ phpunit/class-wp-theme-json-resolver-test.php

[aaronrobertshaw]

I've had another session attempting to ascertain why this test causes the seemingly unrelated test to fail but haven't had any success yet. I'll need to revisit this later this week as there are a few other fixes that need to land in time for WP6.6 beta 3.

[tellthemachines]

I'm not entirely sure what's happening here, but all these tests seem to be using the same block-theme so perhaps the problem can be avoided by creating a new theme specifically to test the block style variations on.

[aaronrobertshaw]

I'm not entirely sure what's happening here, but all these tests seem to be using the same block-theme so perhaps the problem can be avoided by creating a new theme specifically to test the block style variations on.

That's definitely possible. I'd like to understand why it's causing the failure in a seemingly unrelated test to make sure I'm not missing some deeper issue.

It's also worth noting that the block-theme already has a couple of block style variations in it. There's something about this use case. It's also odd that the test passes fine in core.

[andrewserong]

This might be a stretch, but just thinking out loud while testing locally:

The resolver tests include some more detailed cache resets after they're run to ensure that resolver caching is cleared for subsequent tests:

gutenberg/phpunit/class-wp-theme-json-resolver-test.php

Line 67 in 23b2851

public static function set_up_before_class() {

However, the test in this PR is for WP_Block_Supports_Block_Style_Variations_Test, but the test in question uses the resolver class. Do we need to copy over any of the resolver's cleanup logic to ensure the resolver's cache doesn't have any stale values in its static properties, after the block style variations tests are concluded?

I'm mostly wondering this because if I just test the block editor settings class locally, all tests pass for me:

npm run test:unit:php:base -- --filter Gutenberg_REST_Block_Editor_Settings_Controller_Test

However I get the test failure as in CI when I run all the tests:

npm run test:unit:php:base

So it feels like there are stale values happening somewhere here 🤔

[oandregal]

Do we need to copy over any of the resolver's cleanup logic to ensure the resolver's cache doesn't have any stale values in its static properties, after the block style variations tests are concluded?

I gave this a shot and it works — locally. Pushed to see what CI thinks of it.

Quickly looking at what those setup/teardown functions do, it makes sense they are related: clearing the resolver caches make them to be fresh for the next test. For example, block metadata, would no longer hold any WithSlug variation previously registered. Not super familiar with them, so I'd appreciate if anyone gave them a second look and tweak as necessary.

[github-actions]

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: oandregal <oandregal@git.wordpress.org>
Co-authored-by: andrewserong <andrewserong@git.wordpress.org>
Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org>
Co-authored-by: tellthemachines <isabel_brison@git.wordpress.org>
Co-authored-by: ramonjd <ramonopoly@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[andrewserong]

Nice, glad that approach worked! Resetting the cache in this way sounds like the right way to go to me, and it matches what was in the resolver tests. If we run into any issues further down the track, I suppose we could look at also attempting to reset the 'theme' and 'user' properties, but for now, since this gets the tests passing again, I think this looks good to go.

LGTM 🚀

[ramonjd]

I tested with and without b879472 and the caching trick works. Good one @andrewserong !

I'll go ahead and merge this.

I suppose we could look at also attempting to reset the 'theme' and 'user' properties, but for now, since this gets the tests passing again

Was WP_Theme_JSON_Resolver_Gutenberg::clean_cached_data() not an option? It resets core, user, theme and more.

gutenberg/lib/class-wp-theme-json-resolver-gutenberg.php

Line 676 in b879472

public static function clean_cached_data() {

[ramonjd]

Was WP_Theme_JSON_Resolver_Gutenberg::clean_cached_data() not an option? It resets core, user, theme and more.

Seems to work locally (running in tear_down() between tests) - I can create a new PR to test it in CI

[andrewserong]

Was WP_Theme_JSON_Resolver_Gutenberg::clean_cached_data() not an option? It resets core, user, theme and more.

Gosh, that looks like an obvious method to try now!

Feel free to merge this as-is and we can try that as a follow-up. (Or we can push that change and try it now, whichever you prefer).

[ramonjd]

Oh, I just saw that tear_down() has always called _gutenberg_clean_theme_json_caches(), which calls WP_Theme_JSON_Resolver_Gutenberg::clean_cached_data() 🤔 so maybe I'll try in another PR, perhaps in teardown class or something seeing as this one works for now.

Cheers!

[ramonjd]

WP_Theme_JSON_Resolver_Gutenberg::clean_cached_data() 🤔 so maybe I'll try in another PR, perhaps in teardown class or something seeing as this one works for now.

Well, this is fun. That works fine.

But now I've removed the caching changes completely in #62637 and the tests pass on CI and locally. 🤷🏻

[andrewserong]

But now I've removed the caching changes completely in #62637 and the tests pass on CI and locally. 🤷🏻

🤯 that's very confusing 🤔