Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 6 weeks ago

#51154 closed defect (bug) (fixed)

sitemaps should be initialized before each test is run

Reported by: pbiron's profile pbiron Owned by: sergeybiryukov's profile 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 4 years ago.

Download all attachments as: .zip

Change History (13)

#1 @pbiron
4 years ago

  • Version set to 5.5

#2 @SergeyBiryukov
4 years ago

  • Milestone changed from Awaiting Review to 5.5.2

Great investigation, makes sense.

@pbiron
4 years ago

#3 @pbiron
4 years 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 $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
Version 0, edited 4 years ago by pbiron (next)

#4 @pbiron
4 years 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
4 years 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
4 years 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.


4 years ago

#8 @SergeyBiryukov
4 years 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
4 years 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
4 years ago

  • Keywords dev-reviewed added; dev-feedback removed

Looks good to backport.

#11 @desrosj
4 years 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.