sitemaps should be initialized before each test is run

[pbiron]

While working on #50910, both @peterwilsoncc and I came across some odd cases where unit tests were passing/failing when run in isolation but had the opposite result when run as part of a larger group of tests. See #50910:comment:15 and #50910:comment:21.

When handling a normal WP request, sitemaps are initialized on init. As part of that initialization, 3 rewrite tags are added: sitemap, sitemap-subtype and sitemap-stylesheet. That in turn, adds those to $wp->public_query_vars.

However, wp_sitemaps_get_server() (which is hooked to init) checks whether the $wp_sitemaps global is set and if is then sitemaps aren't "re-initialized". During a normal WP request that's desired behavior, but when multiple unit tests are run it can cause incorrect test results.

For example, testPublicQueryVarsAreAsExpected() (in tests/phpunit/tests/query/vars.php) should have been updated as part of the core merge to include the sitemaps-related public_query_vars but it was missed because the test was passing on Travis since sitemaps weren't being reinitialized before each test.

That test can be run by itself with phpunit --group 35115 (it was added as part of #35115, in [36045]) and it will fail...as it should, since it was never updated with the sitemaps-related query vars. But when run as with all the other phpunit --group query tests, it incorrectly succeeds.

So, here's what I propose: I'll open a PR that sets $GLOBALS['wp_sitemaps'] = null; in WP_UnitTestCase_Base::tearDown() (where other globals are reset). When Travis runs, at the very least testPublicQueryVarsAreAsExpected() should fail. I'll then update the PR by adding the sitemap-related query vars to the $expected value in that test.

Well see how that goes and if necessary I'll correct any other tests that have been incorrectly passing on Travis.

[SergeyBiryukov]

Great investigation, makes sense.

[pbiron]

Not sure why, but the PR isn't showing up in the "Pull Requests" box above. For reference, it is ​https://github.com/WordPress/wordpress-develop/pull/502.

And as I expected, testPublicQueryVarsAreAsExpected() failed when $wp_sitemaps was nulled...as it should have. And then when that test was updated to include the sitemaps-related public query vars, it then succeeds again.

51154.diff​ contains both of those changes.

@peterwilsoncc thought maybe this could land in 5.5.1. But I think @SergeyBiryukov is right to delay it to 5.5.2...just to allow a little more time to investigate that:

1. that really was the only problem
2. that where the patch nulls the $wp_sitemaps global is right place that that should be done

[pbiron]

Almost forgot: luckily, that one test seems to be the only one that was incorrectly passing after sitemaps were merged into core. Or at least, it's the only one that uncovered by this fix :-)

[SergeyBiryukov]

Ah, I only set this to 5.5.2 because there was no patch yet :) Since it's only test changes, I think this can go in 5.5.1.

[pbiron]

OK, that's fine with me...5.5.1 it is :-)

[SergeyBiryukov]

In 48908:

Tests: Reset the $wp_sitemap global after each test, so that sitemaps are re-initialized when the next test runs.

This ensures consistent results in query var tests, regardless of whether they are run in isolation or as part of a larger group of tests.

Props pbiron, peterwilsoncc.
Fixes #51154.

[SergeyBiryukov]

Reopening for backporting to the 5.5 branch after a second committer's review.

[desrosj]

Looks good to backport.

[desrosj]

In 48914:

Tests: Reset the $wp_sitemap global after each test, so that sitemaps are re-initialized when the next test runs.

This ensures consistent results in query var tests, regardless of whether they are run in isolation or as part of a larger group of tests.

Props pbiron, peterwilsoncc.
Merges [48908] to the 5.5 branch.
Fixes #51154.

Merged into WP Core in https://core.trac.wordpress.org/changeset/48908