WordPress.org

Make WordPress Core

Opened 5 weeks ago

Last modified 5 weeks ago

#53011 new task (blessed)

Tests: review all setUp() and tearDown()

Reported by: jrf Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords:
Focuses: docs, performance, coding-standards Cc:

Description

The setUpBeforeClass(), setUp(), tearDown() and tearDownAfterClass() (and the WP test suite native variants of these) methods in test classes are used to set up fixtures which will be used by all tests in the test class.

Also see: https://phpunit.readthedocs.io/en/9.5/fixtures.html

For tests to properly be able to test just and only that which is being tested, we need to make sure that changes to the "global state" are undone after each test.

This includes:

  • Changes to variables in $GLOBALS, changes to global variables via global $var_name.
  • Changes to superglobals, like $_GET, $_POST or $_REQUEST.
  • Filters being hooked or unhooked to WordPress using add_filter(), remove_filter(), add_action(), remove_action() (and all their variations) calls in setUp() methods - or in the tests themselves.
  • Changes in the file system - file being created by a test should be deleted after the test.
  • And more contentiously: changes in the database ought to be undone after a test as well. This doesn't mean that any posts being created for a test and only being used by that test need to be deleted, but it does mean that any schema changes, any new tables created etc should be undone after the test.

At this time, the WP Core test suite does not consistently "undo" changes to the global state for each test, as @hellofromTonya and me found while reviewing tests for an unrelated patch.

We would like to recommend a full review of the test suite for best practices regarding global state resetting after each test.

Note: if a "reset" action is being done within the code of a test function, it may not get executed when the tests fails. To that end, these type of "reset"s should generally be done in the tearDown(), even when they only apply to one test in the test class.
The "reset", in that case, should be accompanied by a comment pointing to the test to which it applies.

Change History (3)

#1 @johnbillion
5 weeks ago

Recent work that was done relating to this: [50449], [50450], [50463].

#2 @johnbillion
5 weeks ago

  • Version trunk deleted

#3 @jrf
5 weeks ago

@johnbillion Thanks for linking those changesets. Those are good improvements, glad to see those were already made 💪👍 Looks like those all sprang from #52625.

Note: See TracTickets for help on using tickets.