Fix phpunit failures

[noisysocks]

Fix the phpunit failures in trunk.

All but one of the failures were from Gutenberg phpunit tests that one way or the other call into Core's global styles and settings functionality e.g. wp_get_global_settings. These functions will cache their results unless WP_DEVELOPMENT_MODE is set to 'theme' or if Core detects that the Core unit tests are running via the WP_RUN_CORE_TESTS flag. Since Gutenberg is testing Core functionality here, it is de facto performing the role of Core unit tests, and so the fix is to set WP_RUN_CORE_TESTS. See WordPress/wordpress-develop@4a16702#r119747488.

edit: Nope! All but one of the failures are to do with wp_theme_has_theme_json caching its return value unless WP_RUN_CORE_TESTS is set. See #51950 (comment).

One failure (test_get_template_hierarchy) is due to an issue where a test taxonomy is registered in wpSetUpBeforeClass and not set_up. This causes issues because the switch_theme in set_up will clobber the taxonomies. I note that when these tests were ported to Core this was addressed and we're not using wpSetUpBeforeClass there.

A lot of these tests, I think, don't belong in Gutenberg anymore. They should have been removed when the functionality that they test were moved from Gutenberg to Core. It doesn't make sense to have tests in Gutenberg that test Core functionality. Alternatively, the changes that were made to the PHP test code before being merged to Core should have been brought back to the plugin. For example note how different Tests_Block_Template_Utils looks in Core versus in Gutenberg.

[ramonjd]

Thanks for the help on this one!

[felixarntz]

@noisysocks

These functions will cache their results unless WP_DEVELOPMENT_MODE is set to 'theme' or if Core detects that the Core unit tests are running via the WP_RUN_CORE_TESTS flag.

Hmm, the second part is not correct I think. The only intention of the WP_RUN_CORE_TESTS check in WordPress/wordpress-develop@4a16702#diff-132ca3b683e71f40cf375043fae509e3a541090f428f462e7b68a46480ea83bcR282 is to facilitate overriding the value of the WP_DEVELOPMENT_MODE constant in tests.

As far as I understand, the failing Gutenberg tests assume that the logic is not being cached, which isn't guaranteed. I agree those tests would preferably only live in core, but I would suggest one of the two following workarounds:

• Clear the cache in the relevant tests between calls to the function so there will never be a cache hit.
• Set WP_RUN_CORE_TESTS and then adjust the relevant tests to set the $_wp_tests_development_mode global to 'theme', which is the way that core tests handle this for the case where the logic shouldn't be cached.

I'm a bit confused by the change of just setting WP_RUN_CORE_TESTS to true - that alone shouldn't fix the test failures 🤔

[ramonjd]

Clear the cache in the relevant tests between calls to the function so there will never be a cache hit.

Thanks for the advice @felixarntz

I've made a note to return to this after WP 6.3 Beta 1 (which is soon 😄 )

In the meantime, this patch would unblock a lot of work in the next few days.

@noisysocks do you think we could merge this in the meantime?

[noisysocks]

Thanks for the extra context @felixarntz! You're right, re-reading wp_get_development_mode I can see that WP_RUN_CORE_TESTS shouldn't have any affect. And yet it does... 🤔

I'll try one of your suggestions. We can hold off on merging this unless an urgent need comes up.

[noisysocks]

Ah. Setting WP_RUN_CORE_TESTS fixes the tests because of this check in wp_theme_has_theme_json:

https://github.com/WordPress/wordpress-develop/blob/12ed5ccb0a61cf684682a94e9b9c02dd11bb7d75/src/wp-includes/global-styles-and-settings.php#L405

The tests in Gutenberg are written assuming that WP_DEBUG is enough to stop wp_theme_has_theme_json from caching its result:

gutenberg/lib/compat/wordpress-6.2/get-global-styles-and-settings.php

Line 31 in aaf84f3

if ( ! WP_DEBUG && is_int( $theme_has_support ) ) {

It's not anything to do with wp_get_development_mode.

[noisysocks]

Note also that wp_theme_has_theme_json uses a static variable as the cache and not wp_cache. The unit tests therefore can't manually reset the cache.

I think setting WP_RUN_CORE_TESTS is all that we can do until these tests are removed.

[ramonjd]

I think setting WP_RUN_CORE_TESTS is all that we can do until these tests are removed.

👍

Maybe I'm overthinking it, but say we remove the tests and also the new constant.

What about future additions to say, WP_Theme_Json_Gutenberg, that the plugin will make, experimental or destined for a future Core release. Some of these might need to call wp_theme_has_theme_json() and also require test coverage.

I guess Gutenberg could do what it always does for compatibility, e.g., create a correspsonding, but altered, function gutenbeg_theme_has_theme_json() ?

[noisysocks]

Hmm yeah good flag. We can do what you suggest or, maybe better, update Core's implementation of wp_theme_has_theme_json to cache the return value in a way that is reset by wp_clean_theme_json_cache.

[ramonjd]

update Core's implementation of wp_theme_has_theme_json to cache the return value in a way that is reset by wp_clean_theme_json_cache.

I like the cut of your gib

[felixarntz]

@noisysocks @ramonjd Thank you for the additional digging, based on the further context you provided the fix here LGTM.

Hmm yeah good flag. We can do what you suggest or, maybe better, update Core's implementation of wp_theme_has_theme_json to cache the return value in a way that is reset by wp_clean_theme_json_cache.

Big +1 from me, would you mind opening a Trac ticket for this?

[ramonjd]

Ticket here: https://core.trac.wordpress.org/ticket/58650

Please edit/expand if I've got the details wrong

🙇