WordPress.org

Make WordPress Core

Opened 7 weeks ago

Closed 3 weeks ago

Last modified 3 weeks ago

#53431 closed defect (bug) (fixed)

Tests: Reset global $current_screen between tests to avoid cross-test interdependencies

Reported by: hellofromTonya Owned by: hellofromTonya
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)

#1 @desrosj
7 weeks ago

  • Milestone changed from Awaiting Review to 5.9

#2 @hellofromTonya
7 weeks 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.


7 weeks ago

  • Keywords has-patch has-unit-tests added; needs-patch removed

Trac ticket: https://core.trac.wordpress.org/ticket/53431

Moves the resetting of current screen globals from individual tests to the base test case.

#4 @hellofromTonya
7 weeks 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 @hellofromTonya
6 weeks 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.


3 weeks ago

#7 @SergeyBiryukov
3 weeks ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 51419:

Tests: Reset $current_screen global between tests to avoid cross-test interdependencies.

This provides a consistent global starting state for tests that interact with admin screens.

Individual tests no longer need to invoke set_current_screen( 'front' ) (or an alternative implementation) as a reset.

Follow-up to [29251], [29860], [31046], [36721], [38678], [48908], [50433].

Props hellofromTonya, johnbillion.
Fixes #53431.

Note: See TracTickets for help on using tickets.