WordPress.org

Make WordPress Core

Opened 19 months ago

Last modified 17 months ago

#37207 new defect (bug)

Multiple unit tests for add_rewrite_endpoint lose custom endpoints

Reported by: ericdaams Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 4.5.3
Component: Build/Test Tools Keywords: needs-patch
Focuses: Cc:

Description

When registering multiple endpoints through add_rewrite_endpoint(), tests for the existence of each endpoint in $GLOBALS['wp']->public_query_vars fails.

I have attached a simple plugin that illustrates the problem. Steps to reproduce:

  1. Download the plugin.
  2. Run phpunit inside of it.
  3. Test #1 succeeds while test #2 fails.

The relevant excerpts from the plugin are included below.

Endpoint registration:

add_action( 'init', function() {
	add_rewrite_endpoint( 'custom-endpoint', EP_ROOT );
	add_rewrite_endpoint( 'custom-endpoint-2', EP_ROOT );
});

Tests:

class TestRewriteEndpoint extends WP_UnitTestCase {
	public function setUp() {
		parent::setUp();
		$this->set_permalink_structure( '/%year%/%monthnum%/%day%/%postname%/' );
		$this->qvs = $GLOBALS['wp']->public_query_vars;
		// print_r( $this->qvs );
	}
	public function tearDown() {
		$GLOBALS['wp']->public_query_vars = $this->qvs;
		parent::tearDown();
	}
	public function test_is_custom_endpoint_added() {
		$this->assertContains( 'custom-endpoint', $GLOBALS['wp']->public_query_vars );
	}
	public function test_is_custom_endpoint_2_added() {
		$this->assertContains( 'custom-endpoint-2', $GLOBALS['wp']->public_query_vars );
	}
}

This may possibly be related to #34346, though in this case I am not using go_to().

Attachments (3)

wp-unit-testing-add-rewrite-endpoint-bug-master.zip (4.8 KB) - added by ericdaams 19 months ago.
37207.diff (1.6 KB) - added by mboynes 18 months ago.
Store $wp's query vars after bootstrap to restore on tearDown
37207.2.diff (1.6 KB) - added by mboynes 17 months ago.
Only restore query vars if not running core unit tests

Download all attachments as: .zip

Change History (8)

#1 @mboynes
18 months ago

I ran across this issue today.

In short, it's common practice to call add_rewrite_tag() or add_rewrite_endpoint() on init. These add public query vars to $GLOBALS['wp'] directly (not via the query_vars filter). init fires once on load during a run of phpunit, not once per test. In WP_UnitTestCase::tearDown(), $GLOBALS['wp'] is completely reset, which includes wiping the public query vars. Because init is never run again, those query vars are never added back.

One potential solution would be to store query vars at the end of the bootstrap, then reset them in WP_UnitTestCase::tearDown() after $GLOBALS['wp'] is reset. It would be important to store them on bootstrap and not WP_UnitTestCase::setUp() so that query vars added during one test don't persist to the next test.

#2 @boonebgorges
18 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

One potential solution would be to store query vars at the end of the bootstrap

Something like this seems like the right approach to me.

@mboynes
18 months ago

Store $wp's query vars after bootstrap to restore on tearDown

#3 @mboynes
18 months ago

Potential solution proposed in 37207.diff

#4 @boonebgorges
17 months ago

@mboynes Thanks for the patch. I'm looking it over in more detail, and now I wonder whether it's the right approach.

Consider post types, taxonomies, and post statuses. In each case, it's common practice for plugins to register these object types on init. In #29827, we toyed with a couple different solutions, and it turned out to introduce weird leakage bugs if we tried to back up and then restore global data like this. So we went with [29869], which basically says: when running the *core* tests, the post_type/taxonomy/post_status slate should be wiped clean between tests, but these globals should not be touched between tests when WP_UnitTestCase is called by a plugin.

I'm not certain that this setup is the "best" in any meaningful sense, but it seems like a compromise that's worked for us. Could we consider something similar here? Roughly: the backup/reset process would only happen if ! WP_RUN_CORE_TEST. @mboynes @ericdaams What do you think?

#5 @mboynes
17 months ago

@boonebgorges that approach makes sense to me. I'll update the patch.

@mboynes
17 months ago

Only restore query vars if not running core unit tests

Note: See TracTickets for help on using tickets.