WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#28535 closed enhancement (fixed)

WP Unit Tests should reset hooks after each test

Reported by: mnelson4 Owned by: wonderboymusic
Milestone: 4.0 Priority: normal
Severity: normal Version: 4.0
Component: Build/Test Tools Keywords: needs-patch
Focuses: Cc:

Description

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.

Attachments (1)

ticket-28535.diff (2.6 KB) - added by mnelson4 5 years ago.
patch to backup hooks between each test

Download all attachments as: .zip

Change History (11)

#1 follow-up: @jorbin
5 years ago

  • Keywords needs-patch added

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

#2 @nacin
5 years ago

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.

#3 @nacin
5 years ago

  • Milestone changed from Awaiting Review to 4.0

@mnelson4
5 years ago

patch to backup hooks between each test

#4 @mnelson4
5 years ago

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

#5 in reply to: ↑ 1 @jdgrimes
5 years ago

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.

#6 @wonderboymusic
5 years ago

  • Owner set to wonderboymusic
  • Resolution set to fixed
  • Status changed from new to closed

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.

#7 @DJPaul
5 years ago

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.

#8 @DJPaul
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

#9 @wonderboymusic
5 years ago

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.

#10 @wonderboymusic
5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.