Leftover taxonomies break later tests

[boonebgorges]

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.

[nacin]

Alternative idea, untested: 29827.2.patch​.

[boonebgorges]

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

[jorbin]

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

[boonebgorges]

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.

[boonebgorges]

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

[boonebgorges]

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.

[danielbachhuber]

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.

[danielbachhuber]

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

[boonebgorges]

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

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

[danielbachhuber]

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.

[boonebgorges]

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

[boonebgorges]

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?

[boonebgorges]

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

[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.

[boonebgorges]

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

[boonebgorges]

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.

[nacin]

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.

[boonebgorges]

In 29954:

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

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

See #29827.