WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#29827 closed defect (bug) (fixed)

Leftover taxonomies break later tests

Reported by: boonebgorges Owned by: boonebgorges
Milestone: 4.1 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch commit
Focuses: Cc:

Description

A number of our tests call register_taxonomy(). To make sure the global scope is cleared up for the next test, these tests manually call _unregister_taxonomy(). However, when a test fails before it's had a chance to unregister the custom taxonomy, the global remains polluted. This can break later tests.

The most surefire solution is to reset taxonomies to the default as part of every setUp(). See attached patch. It adds a little overhead, but not much - 2-3% more time to run the suite, in my limited testing. Added benefit: no need to call _unregister_taxonomy() manually.

If we don't like the performance hit, a better-than-nothing solution is to move calls to _unregister_taxonomy() so that they're called before assertions, wherever possible. That way, if the test fails, at least it's cleaned up its mess first.

Attachments (4)

29827.patch (1.2 KB) - added by boonebgorges 6 years ago.
29827.2.patch (1.6 KB) - added by nacin 6 years ago.
29827.3.patch (580 bytes) - added by boonebgorges 6 years ago.
29827.4.patch (1.1 KB) - added by boonebgorges 6 years ago.

Download all attachments as: .zip

Change History (23)

@boonebgorges
6 years ago

@nacin
6 years ago

#1 follow-up: @nacin
6 years ago

Alternative idea, untested: 29827.2.patch.

#2 in reply to: ↑ 1 @boonebgorges
6 years ago

Replying to nacin:

Alternative idea, untested: 29827.2.patch.

Yup, that was my first thought too, but tearDown() isn't run if the test fails. So the taxonomies/post types will still be there for the next test that runs (and for every test that runs until tearDown() is successfully invoked again).

#3 @jorbin
6 years ago

  • Milestone changed from Awaiting Review to 4.1

I think that the extra time in setup is worth the clean slate as it makes our tests less dependent on each other.

#4 @boonebgorges
6 years ago

  • Milestone changed from 4.1 to Future Release
  • Owner set to boonebgorges
  • Status changed from new to accepted

nacin correctly points out in IRC that I am a bozo (paraphrase) and tearDown() does run after an errored/failed test. I cleaned up his 2.patch but it's causing some odd errors in other tests and I've run out of steam for the moment, but I will poke around some more soon and figure something out.

#5 @boonebgorges
6 years ago

  • Milestone changed from Future Release to 4.1

Tore my hair out a bit more over this one.

nacin's suggestion of catching everything on the 'register_taxonomy' and 'register_post_type' hooks won't work. Some test classes call create_initial_taxonomies(), and it's likely that other tests do weird things where they reregister builtin post types. When that happens, and they get added to the $registered_taxonomy/post_type list, they later get unregistered, which is not what we want.

So, I tried something like this: during setUp(), record all existing taxonomies, then during tearDown(), unregister array_diff( $existing, get_taxonomies() ). But this causes problems that are similar to (though a bit less pervasive than) nacin's suggestion. I started to look though the tests to see where they were doing messy stuff, but that's such an enormous task that I'd rather avoid it.

29827.3.patch goes back to the initial suggestion, and adds support for post types, which requires a rewrite rule flush. This is pretty foolproof, and speed difference is pretty negligible.

This change caused one error in a test (test_get_the_taxonomies()), which can be traced back to the fact that create_initial_taxonomies() does not explicitly declare 'hierarchical=false' for 'post_tag', and register_taxonomy() only sets this key if you are using a non-default permastruct. I'm fairly sure this is a bug, so patch attached, but this could use the eyes of someone more familiar with how rewrite rules should be regenerated between tests. (Note that the PHPUnit error only occurs when you run the whole suite - so comment out my taxonomy.php fix and run it to see what I'm talking about.)

#6 @boonebgorges
6 years ago

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

In 29860:

Reset post types and taxonomies before each unit test.

Registering a post type or taxonomy during a unit test causes modifications to
global variables. If the test fails to clean up these globals - either by
neglecting to call _unregister_post_type()/_unregister_taxonomy() at all or by
failing before getting a chance to do so - tests that run later in the suite
can fail, leading to much gnashing of teeth. Wiping all taxonomies and
restoring to the defaults before each test ensures that we always start with a
clean slate.

Fixes #29827.

#7 @danielbachhuber
6 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

For those of us using core's testing framework for our own plugins, themes, etc., r29860 breaks any testing of custom taxonomies and post types.

Calling do_action( 'init' ); again generally causes the universe to collapse in a black hole.

#8 @danielbachhuber
6 years ago

With this being said, I don't think I'm actually against the change, just wanted to flag that it will break a lot of things for a lot of people. It's easy enough to re-register post types and taxonomies in the setUp() method

#9 @boonebgorges
6 years ago

danielbachhuber - Can you share a specific example so I can have a closer look?

We may think about moving the reset routines to tearDown().

#10 @danielbachhuber
6 years ago

Can you share a specific example so I can have a closer look?

My immediate example is from a private client repo, so I've emailed you a screenshot.

Essentially, the configuration is this:

  1. Repo comprises a custom theme.
  2. Bootstrap file uses a filter on pre_option_stylesheet and pre_option_template to load the theme.
  3. Tests extend WP_UnitTestCase.
  4. Theme defines post types and taxonomies on init.
  5. Theme also loads Co-Authors Plus, which defines a taxonomy and post type.
  6. First test passes because the init hook fired and registers everything. Subsequent tests failboat in basically every way they can because the custom post types and taxonomies are no longer registered.

Re-registering everything in setUp() (well, calling the methods that do) fixes things cleanly, but it's a bit tedious.

#11 @boonebgorges
6 years ago

  • Status changed from reopened to accepted

OK. It's definitely not desirable to require others to redo their whole setup in setUp(). Moving everything to tearDown() will probably solve the problem, but I want to test this with a skeleton project. If you get a chance and want to provide this, it'd be helpful :)

#12 @boonebgorges
6 years ago

Moving everything to tearDown() will probably solve the problem, but I want to test this with a skeleton project.

On further thought, this is pretty obviously not true (that's what I get for posting when I roll out of bed). 'init' is fired just once, before all the tests are run. It's not going to matter when our testcase tears everything down - as soon as it's done, everything run at 'init' is going to get reset.

In theory, each test should be running with a totally clean slate. If we could roll back everything that's touched on 'init' and then refire the action before each test, that would actually be ideal. And with some work, we could probably make this work for WP itself, since we're aware of all the stuff that gets touched during our own bootstrap. This is not the case for other plugins - they may be setting globals etc that WP can't know about. So, to some extent, tasks related to cleanup (and re-setup) must of necessity be left to plugin test suites. This suggests that this is largely a developer education issue.

That being said, if there were a way for our suite to be a little more generous to devs, I'd be willing to look at it. Let's say, for instance, that reset_post_types() and reset_taxonomies() only ran when the WP core tests were being run. Maybe, say, we'd detect whether a test class has WP_UnitTestCase (or WP_XMLRPC_UnitTestCase or WP_Ajax_UnitTestCase - in other words, a core testcase) as a *direct* parent before resetting these globals; tests that have core testcases as further ancestors (like BP_UnitTestCase https://buddypress.trac.wordpress.org/browser/tags/2.1.1/tests/phpunit/includes/testcase.php) would not. This is less than ideal if we're trying to force devs to "do the right thing", but it will cause less breakage for third parties.

danielbachhuber, as one of the intrepid souls extending the core tests in this way, do you have thoughts on the above?

This ticket was mentioned in IRC in #wordpress-dev by boonebgorges. View the logs.


6 years ago

#14 @boonebgorges
6 years ago

  • Keywords has-patch added

29827.4.patch is a much simpler fix. Basically, only wipe+reset post types and taxonomies if running phpunit from the WP root. Other projects that use WP_UnitTestCase or whatever won't have WP_RUN_CORE_TESTS enabled by default, though they could flip it on if they wished. (I chose a generic constant name because this could be useful elsewhere, but it could be made more specific.)

So, the question is really whether it's more important (a) to enforce separation of tests for anyone using our test tools (in which case, wontfix), or (b) not to break every CI build that depends on WP test tools (in which case, 4.patch).

#15 follow-up: @jorbin
6 years ago

  • Keywords commit added; 2nd-opinion removed

In order to encourage more and better unit tests, I think it's important to not break plugin and theme tests that are based off of WP_UnitTestCase. 29827.4.patch looks like a good solution to me.

#16 in reply to: ↑ 15 @boonebgorges
6 years ago

Replying to jorbin:

In order to encourage more and better unit tests, I think it's important to not break plugin and theme tests that are based off of WP_UnitTestCase. 29827.4.patch looks like a good solution to me.

Thanks, jorbin.

How's this for a Delightful Anecdote: I was working on a client project this afternoon, saw my build was failing, and spent 15 minutes debugging before I realized that I was having the same problem as danielbachhuber. https://www.youtube.com/watch?v=DhrfhjLd9e4

#17 @boonebgorges
6 years ago

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

In 29869:

Only reset taxonomies and post types between tests when running core tests.

A growing number of plugins and other WP-based projects use the core test
tools, such as WP_UnitTestCase, as the basis of their own tests and continuous
integration setups. At the same time, many of these third-party plugins use
custom post types and taxonomies, which are generally registered a single time
during a run of the tests: at 'init', before the testcases have run. Wiping out
these globals between tests will mess with these third-party builds.

Best practice for plugin developers is probably to clean up their own post types
and taxonomies and then reinitialize before each test. But, in the interest of
not breaking everyone's builds, the core test suite will not enforce this.

Fixes #29827.

#18 @nacin
6 years ago

I still like 29827.2.patch as a potential (future) solution. As boone and I discussed in IRC, tearDown() *is* called on test failure, though he said he was having other intermittent issues with this solution.

#19 @boonebgorges
6 years ago

In 29954:

Ensure that post types and taxonomies are reset between multisite tests.

This is a port of [29869] to multisite.xml.

See #29827.

Note: See TracTickets for help on using tickets.