#53431 closed defect (bug) (fixed)
Tests: Reset global $current_screen between tests to avoid cross-test interdependencies
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| Milestone: | 5.9 | Priority: | normal |
| Severity: | normal | Version: | |
| Component: | Build/Test Tools | Keywords: | has-patch has-unit-tests commit |
| Focuses: | Cc: |
Description
As discovered in #52607, the global state of current_screen is not being reset to a known and expected state between tests. Rather, it's left to the individual test that modifies the state to reset it.
Why is this a problem?
If the global state is not consistent when a test starts, it can have unexpected results such as:
- unstable test(s) downstream
- misleading information for a specific piece of functionality under test which could result in "fixing" a problem that doesn't exist in the codebase but instead is in the test suite
- increased effort to debug where the state was changed
- bad contributor experience
Other problems:
- Increases the amount of code in the test suite as each test has to handle setting the state and then resetting it when done
- Inconsistencies in how resetting is done (i.e. in the implementation)
- Inconsistencies in the state's value when resetting (see above for the problems this can cause)
- Reset circuit not being reached when a test fails and the reset is after the failed assertion
This ticket proposes to move the global resetting to the test framework's abstract base test case WP_UnitTestCase_Base and likely in its tearDown method.
Change History (8)
#2
@
4 years ago
- Keywords needs-patch added
- Owner set to hellofromTonya
- Status changed from new to assigned
This ticket was mentioned in PR #1380 on WordPress/wordpress-develop by hellofromtonya.
4 years ago
#3
- Keywords has-patch has-unit-tests added; needs-patch removed
#4
@
4 years ago
PR 1380 does the following:
- Resets the globals associated with current screen in
WP_UnitTestCase_Base::tearDown() - Removes the resetting from the individual tests (I think I got them all)
How is doing the reset?
There were a few approaches to resetting, but the most popular was:
set_current_screen( 'front' );
What does this do?
- It instantiates a new
WP_Screenobject - Sets global
$current_screento the instance - Sets global
$taxnowto an empty string - Sets global
$typenowto an empty string - Fires
'current_screen'action event passing the instance to callbacks
I wondered: Why is 'front' needed as a starting state? Is it needed?
- Looking at the other resets in the base test case, they are set to
null. Tried it for current screen globals after removing resets from individual tests. Tests all passed. - How about unsetting the globals? Tests all passed.
Alrighty, 'front' isn't needed. Cool.
Which approach did the PR implement?
- Setting the 3 globals to
null - Why? Consistency with the other resets
@SergeyBiryukov @johnbillion What do you think about this reset approach?
$current_screen_globals = array( 'current_screen', 'taxnow', 'typenow' );
foreach ( $current_screen_globals as $global ) {
$GLOBALS[ $global ] = null;
}
#5
@
4 years ago
- Keywords commit added
@johnbillion has reviewed and approved. Marking for commit consideration.
This ticket was mentioned in Slack in #core-test by hellofromtonya. View the logs.
4 years ago
hellofromtonya commented on PR #1380:
4 years ago
#8
Committed in changeset 51419
Trac ticket: https://core.trac.wordpress.org/ticket/53431
Moves the resetting of current screen globals from individual tests to the base test case.