WP Unit Tests should reset hooks after each test

[mnelson4]

After each unit test method is ran (Eg TEsts_Actions_Callbacks::test_callback_representations() ), the actions and filters set during that unit test preserved. This means that any filters added during one unit test will affect subsequent tests (which is troublesome because you can then have tests that fail because a hook was set on a previous test, but if you only run the unit test that failed (without the previous test that added the hook), it may pass.)

I'm thinking the fix would be:
add a global called something like $saved_wp_actions; During WP_UnitTestCase::setUp() copy the contents of $wp_actions to it. Then during WP_UnitTestCase::tearDown() restore $wp_actions to its previous state from before the test by assigning it to be equal to $saved_wp_actions.

[jorbin]

This seems like a really good idea to me. We can potentially use the phpunit option ​backupGlobals , but I know @nacin mentioned to me once that we tried this and it caused issues. I'm not sure if that is still the case. It's worth investigating.

If this can get a good patch soon, I think it would make sense to include this in 4.0

[nacin]

Note we'd need to back up $wp_filter as well. The actions array holds counters on what's been run; $wp_filter does most of the work.

[mnelson4]

patch to backup hooks between each test

[mnelson4]

Good point @nacin. I actually also backed-up $merged_filters and $wp_current_filter.

@jorbin ya using phpunit's backupGlobals config option might be a good idea. And even if there are any legitimate uses of globals that SHOULD NOT be reset between each test, those can probably be overcome by ​blacklisting them, and using the annotations like you linked to.

I wonder what the issues were about why we'd want to avoid backing up globals? It sounds even better than what I've done because other globals wouldn't be carrying over from one test to the next...

[jdgrimes]

Replying to jorbin:

This seems like a really good idea to me. We can potentially use the phpunit option ​backupGlobals , but I know @nacin mentioned to me once that we tried this and it caused issues. I'm not sure if that is still the case. It's worth investigating.

More docs on backupGlobals: ​http://phpunit.de/manual/current/en/fixtures.html#fixtures.global-state

I don't think we'll be able to use it though. When I try to run a single test I mocked up, with backupGlobals enabled it takes forever (I didn't wait to see how long it would take to actually run, I killed it after several minutes). (And yes, it runs fine with backupGlobals disabled.) I think the reason for this is that WordPress's global scope is too heavy.

[wonderboymusic]

In 29251:

Backup filter globals ( $merged_filters, $wp_actions, $wp_current_filter, $wp_filter ) statically when running unit tests, restore on tearDown(). This ensures that all tests initially use the same filters/actions.

Props mnelson4, wonderboymusic.
Fixes #28535.

[DJPaul]

Regarding r29251; the BuddyPress team has noticed that with PHP 5.2, this change causes PHP to segfault when running unit tests on classes that have a __destruct method and a static class property. For the play-by-play, read ​https://buddypress.trac.wordpress.org/ticket/5803#comment:3 onwards.

This issues seems to only occur in PHP 5.2; we've been testing with 5.2.17. It seems to be fixed in 5.3+. The PHP bug was originally reported at ​https://bugs.php.net/bug.php?id=51822.

We've "fixed" our tests by skipping the tests that are triggering this bug when they are run in a PHP < 5.3 environment.

[DJPaul]

Re-opening so this isn't missed, as it is strictly speaking a regression in the test suite.

[wonderboymusic]

There are 2 conventions for removing filters in WP unit tests:

1. on ->tearDown()
2. when the filter callback is invoked, the first line of the callback immediately removes it

There is no precedent for using __destruct() in the WordPress suite, so it's only a BuddyPress suite regression. Because it causes issues in 5.2.x only, why not standardize on not using __destruct() and call it a day?

Our only other option is to introduce more globals to store the values, which I view as a worse solution.