#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_Screen
object - Sets global
$current_screen
to the instance - Sets global
$taxnow
to an empty string - Sets global
$typenow
to 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.