WordPress.org

Make WordPress Core

Opened 3 weeks ago

Closed 4 hours ago

#42044 closed defect (bug) (fixed)

_test_list_hierarchical_page() - improve clean-up

Reported by: birgire Owned by: SergeyBiryukov
Milestone: 4.9 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch
Focuses: Cc:

Description

The _test_list_hierarchical_page() helper method modifies $_REQUEST and $GLOBALS, but does not unset it afterwards.

It affected a new test I was writing in a strange way, see #40188.

There might be other test methods, that modify globals, without a clean-up afterwards.

Attachments (2)

42044.patch (534 bytes) - added by birgire 3 weeks ago.
Clean up for $_REQUEST and $GLOBALS set in Tests_Admin_includesListTable::_test_list_hierarchical_page()
42044.2.patch (828 bytes) - added by birgire 3 weeks ago.

Download all attachments as: .zip

Change History (7)

@birgire
3 weeks ago

Clean up for $_REQUEST and $GLOBALS set in Tests_Admin_includesListTable::_test_list_hierarchical_page()

#1 @swissspidy
3 weeks ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 4.9
  • Version trunk deleted

Thanks @birgire, good catch!

The only problem with 42044.patch is that this won't run when the assertion above fails. Execution is stopped there. Putting this after $output = ob_get_clean(); instead would prevent this.

@birgire
3 weeks ago

#2 @birgire
3 weeks ago

Thank you @swissspidy, good point

I updated the patch in 42044.2.patch

#3 @birgire
3 weeks ago

We have backupGlobals="false" by default.

I played with the @backupGlobals annotation on the test class:

/**
 * @group admin
 * @backupGlobals enabled
 */
class Tests_Admin_includesListTable extends WP_UnitTestCase {
        protected $backupGlobalsBlacklist = array( 'wpdb' );

where we blacklist the $wpdb object, as it seems it can't survive the serialization.

The blacklisting is explained here.

I ran the

tests/phpunit/tests/admin/includesListTable.php

three times, without the annotation:

Time: 1.43 seconds, Memory: 34.00MB
OK (14 tests, 32 assertions)

Time: 1.51 seconds, Memory: 34.00MB
OK (14 tests, 32 assertions)

Time: 1.39 seconds, Memory: 34.00MB
OK (14 tests, 32 assertions)

using the 42044.2.patch ] and three times, only with the @backupGlobals:

Time: 1.63 seconds, Memory: 38.00MB
OK (14 tests, 32 assertions)

Time: 1.59 seconds, Memory: 38.00MB
OK (14 tests, 32 assertions)

Time: 1.56 seconds, Memory: 38.00MB
OK (14 tests, 32 assertions)

So there's some overhead involved with it.

It didn't seem to have any effect to only enabled it on the problematic method:

/**
* Helper function to test the output of a page which uses `WP_Posts_List_Table`.
*
* @backupGlobals enabled
* @param array $args         Query args for the list of pages.
* @param array $expected_ids Expected IDs of pages returned.
*/
protected function _test_list_hierarchical_page( array $args, array $expected_ids ) {

with

FAILURES!
Tests: 14, Assertions: 31, Failures: 2.

Last edited 3 weeks ago by birgire (previous) (diff)

#4 @birgire
3 weeks ago

ps: I just searched the trac and found 4 tickets where backupGlobals are discussed:

https://core.trac.wordpress.org/search?q=backupGlobals

in e.g. #28535 it was mentioned as worth investigating, but had previously caused issues and that using it would probably be to heavy for the whole project.

Not sure how the status is today, but I can imagine, from the above testing, that it would at least increase the memory usage and the run-time. That might overshadow the positive things it could have ;-)

ps: I found two files where the preserveGlobalState annotation is used:

tests/phpunit/tests/oembed/headers.php: * @preserveGlobalState disabled
tests/phpunit/tests/ajax/Response.php:   * @preserveGlobalState disabled

Last edited 3 weeks ago by birgire (previous) (diff)

#5 @SergeyBiryukov
4 hours ago

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

In 41929:

Build/Test Tools: Clean up $_REQUEST and $GLOBALS after modifying them in _test_list_hierarchical_page().

Props birgire.
Fixes #42044.

Note: See TracTickets for help on using tickets.