WordPress.org

Make WordPress Core

Opened 3 weeks ago

Closed 3 weeks ago

#51154 closed defect (bug) (fixed)

sitemaps should be initialized before each test is run

Reported by: pbiron Owned by: SergeyBiryukov
Milestone: 5.5.1 Priority: normal
Severity: normal Version: 5.5
Component: Build/Test Tools Keywords: has-patch commit dev-reviewed fixed-major
Focuses: Cc:

Description

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.

Attachments (1)

51154.diff (1.8 KB) - added by pbiron 3 weeks ago.

Download all attachments as: .zip

Change History (12)

#1 @pbiron
3 weeks ago

  • Version set to 5.5

#2 @SergeyBiryukov
3 weeks ago

  • Milestone changed from Awaiting Review to 5.5.2

Great investigation, makes sense.

@pbiron
3 weeks ago

#3 @pbiron
3 weeks ago

  • Keywords has-patch added

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
Last edited 3 weeks ago by pbiron (previous) (diff)

#4 @pbiron
3 weeks ago

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 :-)

#5 @SergeyBiryukov
3 weeks ago

  • Milestone changed from 5.5.2 to 5.5.1

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.

#6 @pbiron
3 weeks ago

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

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


3 weeks ago

#8 @SergeyBiryukov
3 weeks ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

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.

#9 @SergeyBiryukov
3 weeks ago

  • Keywords commit dev-feedback fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

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

#10 @desrosj
3 weeks ago

  • Keywords dev-reviewed added; dev-feedback removed

Looks good to backport.

#11 @desrosj
3 weeks ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

Note: See TracTickets for help on using tickets.