WordPress.org
Get WordPress

Make WordPress Core

Opened 2 years ago

Closed 19 months ago

Last modified 19 months ago

#56706 closed defect (bug) (fixed)

Tests: `parent::set_up()` calls `wp_list_pluck()`, causing inaccurate coverage.

Reported by: costdev's profile costdev Owned by: hellofromtonya's profile hellofromTonya
Milestone: 6.2 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-screenshots has-patch commit has-unit-tests
Focuses: Cc:

Description (last modified by costdev)

How was this discovered?

While adding new tests for WP_List_Util::pluck(), I noticed that the coverage report included coverage that should not have been possible.

Steps to reproduce

  1. Add the following to your PHPUnit configuration file, either phpunit.xml.dist, or copy this to phpunit.xml. 
<coverage includeUncoveredFiles="true" processUncoveredFiles="false" pathCoverage="true" cacheDirectory="./tests/phpunit/build/logs/phpunit-cache">
    <include>
        <directory suffix=".php">src/wp-includes/class-wp-list-util.php</directory>
    </include>
    <report>
        <text outputFile="php://stdout" showOnlySummary="true"/>
        <html outputDirectory="./tests/phpunit/build/logs/coverage-html"/>
    </report>
</coverage>
  1. Add the following test method to tests/phpunit/tests/functions/wpListPluck.php. 
<?php

/**
 * @covers WP_List_Util::pluck
 */
public function test_set_up_causes_wp_list_util_pluck_coverage_bug() {
        $this->assertTrue( true );
}
  1. Run: 
    XDEBUG_MODE=coverage phpunit --filter test_set_up_causes_wp_list_util_pluck_coverage_bug
  1. Navigate to: 
    http://localhost/wordpress-develop/tests/phpunit/build/logs/coverage-html/class-wp-list-util.php.html
  1. Click pluck.
  2. 🐞 Observe that parts of this method are unexpectedly covered.

What's happening?

Huge thanks to @jrf for research on this!

  • WP_UnitTestCase_Base::set_up() calls WP_UnitTestCase_Base::reset_post_statuses().
  • Which calls get_post_stati().
  • Which calls wp_filter_object_list().
  • Which calls WP_List_Util::pluck(). 💥

Since code called in WP_UnitTestCase_Base::set_up() is included in coverage reports, this presents an inaccurate coverage report for WP_List_Util::pluck().

Solutions

The real solution involves ensuring that the code under test is not called in WP_UnitTestCase_Base::set_up(). However, this is best done after a wider discussion on improving the test suite.

For now, removing parent::set_up() from Tests_Functions_wpListPluck::set_up() will prevent inaccurate coverage and confusion for contributors. To allow for testing deprecation and incorrect usage notices, $this->expectDeprecated() should be added instead.

Attachments (3)

WP_List_Util.pluck.inaccurate.coverage.jpg​ (67.6 KB) - added by costdev 2 years ago.
56706.diff​ (941 bytes) - added by costdev 2 years ago.
56706.2.diff​ (1.5 KB) - added by SergeyBiryukov 2 years ago.

Download all attachments as: .zip

Change History (17)

@costdev
2 years ago

@costdev
2 years ago

#1 @costdev
2 years ago

  • Description modified (diff)

#2 @jrf
2 years ago

  • Keywords commit added

Note: the above reproduction scenario needs PHPUnit 9.3+ to run (due to the use of the <coverage> element in the configuration).

Other than that, I can confirm what @costdev described and would recommend for this patch to be committed.

Loosely related to #53011, #53746, #39265

@SergeyBiryukov
2 years ago

#3 follow-up: @SergeyBiryukov
2 years ago

Thanks for the ticket!

Would rewriting reset_post_statuses() to avoid using get_post_stati() be a better solution here?

At a glance, it looks like reset_post_types() and reset_taxonomies() would also need to avoid using get_post_types() and get_taxonomies(), respectively, as they call wp_filter_object_list() as well.

See 56706.2.diff​.

#4 in reply to: ↑ 3 ; follow-up: @jrf
2 years ago

Replying to SergeyBiryukov:

Would rewriting reset_post_statuses() to avoid using get_post_stati() be a better solution here?

At a glance, it looks like reset_post_types() and reset_taxonomies() would also need to avoid using get_post_types() and get_taxonomies(), respectively, as they call wp_filter_object_list() as well.

For what's it's worth - the same problem as outlined in this ticket applies to any and all tests involving WP native functions used in the set_up() methods (and possibly the tear_down(), though I'd need to check to be sure).

So swopping out one WP native function for another is not a solution, it just "moves" the problem to another part of the test suite.

More than anything, we need a different setup for tests which don't need the whole WP environment to be up and running, but there are other tickets open for that.

#5 in reply to: ↑ 4 ; follow-up: @SergeyBiryukov
2 years ago

Replying to jrf:

For what's it's worth - the same problem as outlined in this ticket applies to any and all tests involving WP native functions used in the set_up() methods (and possibly the tear_down(), though I'd need to check to be sure).

Thanks! At a glance, some other native WP functions used in WP_UnitTestCase_Base::set_up() appear to be:

  • add_filter()
  • add_action()
  • wp_upload_dir()
  • WP_Rewrite::init()
  • WP_Rewrite::set_permalink_structure()
  • WP_Rewrite::flush_rules()

So swopping out one WP native function for another is not a solution, it just "moves" the problem to another part of the test suite.

I might be missing something, but 56706.2.diff​ does not replace wp_filter_object_list() with any other native WP function, it just uses the globals directly. It was an attempt to implement this suggestion from the ticket description:

The real solution involves ensuring that the code under test is not called in WP_UnitTestCase_Base::set_up().

#6 in reply to: ↑ 5 @jrf
2 years ago

Replying to SergeyBiryukov:

So swopping out one WP native function for another is not a solution, it just "moves" the problem to another part of the test suite.

I might be missing something, but 56706.2.diff​ does not replace wp_filter_object_list() with any other native WP function, it just uses the globals directly. It was an attempt to implement this suggestion from the ticket description:

The real solution involves ensuring that the code under test is not called in WP_UnitTestCase_Base::set_up().

Appreciated, My comment wasn't directly aimed at your patch, but the problem with using globals is, of course, that you need to be sure those globals have not been adjusted in the tests and if they have, that those globals will need to be reset after each test again too.

To be honest, I think we need to leave the more structural solution to #53011, #53746 and the more structural test suite refactor and "all test review" in #53010.

My line of thinking for a more structural solution would be to have a very "plain" base abstract TestCase which only contains the WP native assertions and expectation handling.

And then have a number of abstract TestCases which can be used for specific test situations, something like CleanDatabaseTestCase, EmptyCacheTestCase etc (just some ideas, the real TestCases needed would need to be determined based on a detailed analysis of the tests)

Alternatively, it could be that the base TestCase only offers utility functions to do certain "set up" and "clean up" tasks and that each test class would need to determine what is the most suitable combination for those tests.

More than anything, the base set_up() and tear_down() methods need some very careful scrutiny as they are just doing too much and doing it wrong (tear down tasks in set up and visa versa) and for the tests of a lot of functions using mostly plain PHP, this amount of set up and tear down is just not needed and is detrimental.

#7 @audrasjb
2 years ago

  • Keywords commit removed
  • Milestone changed from 6.1 to 6.2

With WP 6.1 RC 1 scheduled today (Oct 11, 2022), there is not much time left to address this ticket. Let's move it to the next milestone since it appears it is still being discussed.

Ps: if you were about to send a patch and if you feel it is realistic to commit it in the next one or two hours, please feel free to move this ticket back to milestone 6.1.

#8 @hellofromTonya
19 months ago

  • Keywords commit added
  • Owner changed from costdev to hellofromTonya
  • Status changed from assigned to reviewing

Reviewed the discussions. Remarking 56706.diff​ for commit to resolve the reported issue while staying in scope for resolving just this one issue.

Self-assigning for commit.

This ticket was mentioned in ​PR #4074 on ​WordPress/wordpress-develop by ​@hellofromTonya.
19 months ago #9

  • Keywords has-unit-tests added

56706.diff props costdev

This PR is intended to run 56706.diff against all of the CI jobs before commit.

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

​@hellofromTonya commented on ​PR #4074:
19 months ago #10

Comparing the CI jobs to ​the lastest run on `trunk`, there are no differences or impacts from this change ✅

Cool, this is ready for commit 👍

#11 @hellofromTonya
19 months ago

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

In 55341:

Build/Test Tools: Fix inaccurate coverage in Tests_Functions_wpListPluck::set_up().

The call stack for WP_UnitTestCase_Base::set_up() includes a call to WP_List_Util::pluck(), which creates an inaccurate coverage report for this method.

To resolve, parent::set_up() is removed from Tests_Functions_wpListPluck::set_up().

To ensure that deprecation and incorrect usage notices continue to be detectable, $this->expectDeprecated() is added in the test's set_up() fixture.

Follow-up to [51663], [28900].

Props costdev, jrf, SergeyBiryukov, audrasjb.
Fixes #56706.

#13 @hellofromTonya
19 months ago

Hey @SergeyBiryukov, are you okay with not committing 56706.2.diff​ in this ticket, per what @jrf explained? Checking to ensure there's consensus to now close this ticket.

#14 @SergeyBiryukov
19 months ago

Replying to hellofromTonya:

are you okay with not committing 56706.2.diff​ in this ticket, per what @jrf explained?

Yes, fair enough :) Thanks for the commit! Appreciate Juliette's detailed explanation too.

Note: See TracTickets for help on using tickets.