Make WordPress Core

Opened 7 years ago

Closed 3 years ago

Last modified 3 years ago

#38266 closed task (blessed) (fixed)

Tests: Use assertSame() when the type of the value in the assertion is important

Reported by: johnbillion's profile johnbillion Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.7 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: needs-unit-tests php8
Focuses: Cc:

Description

Several tests use assertEquals() on a falsey value where the type is important to the accuracy of the test.

For example, in Tests_Cache::test_miss(), a value is tested against an expected NULL value using assertEquals() which will produce a false positive if another falsey value such as 0, false, or an empty string is passed. These means these such assertions are actually buggy.

Using assertSame() instead of assertEquals() will cause the type to be checked in addition to its value.

Change History (36)

#1 @johnbillion
7 years ago

Looks like two of the assertions in Tests_Cache are incorrect.

  1. test_miss() - false is returned from a cache miss, not null.
  2. test_add_get_null() - a null value is retained, not converted to an empty string.

These will need more investigation.

#2 @johnbillion
5 years ago

  • Keywords needs-unit-tests added; ongoing removed

#3 @johnbillion
5 years ago

I wrote a package to detect usage of assertEquals() on falsey values: https://github.com/johnbillion/falsey-assertequals-detector. If someone wants to pick this up, that package could be used to start addressing these cases in core.

This ticket was mentioned in Slack in #core-multisite by johnbillion. View the logs.


4 years ago

#5 @jrf
4 years ago

Noting here that this will become all the more important with PHP 8 introducing stricter type check exceptions. Documenting via the tests what the actual type of a expected return is + strict checking the type by using assertSame(), will be extremely helpful to detect more PHP 8 related problems.

#6 @SergeyBiryukov
4 years ago

  • Milestone changed from Future Release to 5.6
  • Owner set to SergeyBiryukov
  • Status changed from new to accepted

Wanted to create a ticket for this, but found this one instead and would like to pick it up :)

In the same way that it is considered best practice per the coding standards to use strict comparisons, I think it would be beneficial to switch most of the tests to use assertSame() instead of assertEquals() where possible.

#7 @SergeyBiryukov
4 years ago

  • Keywords php8 added

#8 @SergeyBiryukov
4 years ago

In 48937:

Tests: First pass at using assertSame() instead of assertEquals() in most of the unit tests.

This ensures that not only the return values match the expected results, but also that their type is the same.

Going forward, stricter type checking by using assertSame() should generally be preferred to assertEquals() where appropriate, to make the tests more reliable.

Props johnbillion, jrf, SergeyBiryukov.
See #38266.

#9 @jrf
4 years ago

@SergeyBiryukov My HERO!!! <3

This ticket was mentioned in Slack in #core by sergey. View the logs.


4 years ago

#11 @SergeyBiryukov
4 years ago

In 48939:

Tests: Introduce assertSameSets() and assertSameSetsWithIndex(), and use them where appropriate.

This ensures that not only the array values being compared are equal, but also that their type is the same.

These new methods replace most of the existing instances of assertEqualSets() and assertEqualSetsWithIndex().

Going forward, stricter type checking by using assertSameSets() or assertSameSetsWithIndex() should generally be preferred, to make the tests more reliable.

Follow-up to [48937].

See #38266.

#12 @SergeyBiryukov
4 years ago

In 48940:

Tests: Replace a few more instances of assertEquals() with assertSame().

These were previously missed due to incorrect capitalization.

Follow-up to [48937], [48939].

See #38266.

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


4 years ago

#14 @SergeyBiryukov
4 years ago

In 48944:

Tests: Replace a few more instances of assertEquals() with assertSame().

These were previously missed due to incorrect capitalization.

Follow-up to [48937], [48939], [48940].

See #38266.

#15 @SergeyBiryukov
4 years ago

In 48948:

Tests: Correct assertion in Tests_Cache::test_miss().

On failure to retrieve contents, WP_Object_Cache::get() returns false, not null.

The test only passed accidentally due to assertEquals() not performing a strict type check.

Props johnbillion.
See #38266.

#16 @SergeyBiryukov
4 years ago

  • Tests_Cache::test_miss() was added in [1/tests]. Object caching was introduced in [3011]. As far as I can tell, WP_Object_Cache::get() always returned false, not null for cache misses, so the test was never correct.
  • Tests_Cache::test_add_get_null() was added in [280/tests] and adjusted in [327/tests]. At the time, null was indeed converted to an empty string due to this piece in WP_Object_Cache::set(), added in [3021] and adjusted in [6247]:
    if (NULL === $data)
    	$data = '';
    
    This was later removed in [20089], but the test was never updated.

#17 @SergeyBiryukov
4 years ago

In 48949:

Tests: Correct assertion in Tests_Cache::test_add_get_null().

It is possible to store null in the cache without it being converted to an empty string.

Follow-up to [20089].

Props johnbillion, SergeyBiryukov.
See #38266.

#18 @SergeyBiryukov
4 years ago

In 48950:

Tests: Add a test case for storing false in the cache.

Follow-up to [20089], [48949].

See #38266.

#19 follow-up: @SergeyBiryukov
4 years ago

The commits here so far have reduced the number of assertEquals() instances from 8378 matches in 431 files (WordPress 5.5) to 572 matches in 121 files.

The remaining instances fall into three groups:

  • Ones that use delta comparison for floats, dates, etc. These should be switched to assertEqualsWithDelta() once PHPUnit 7.5 is the minimum supported version. Or, perhaps assertEqualsWithDelta() should be polyfilled for older PHPUnit versions.
  • Ones that legitimately use assertEquals() for comparing objects. Trying to switch these to assertSame() would result in "Failed asserting that two variables reference the same object" error.
  • Ones that should be switched to assertSame(), but would currently result in a failure when doing so, due to data type mismatches, mostly strings vs. integers, or integers vs. floats. These need further investigation:
    • In some cases, the tests should be adjusted to use the correct data type.
    • In other cases, this points to minor bugs in core, e.g. using ceil() for values that are documented as integers, but without explicitly casting to int, results in them being of type float instead. This affects some properties like max_num_pages, max_pages, total_pages in various classes, or functions like get_comment_pages_count(), wp_embed_defaults(), get_oembed_response_data().
Last edited 4 years ago by SergeyBiryukov (previous) (diff)

#20 @SergeyBiryukov
4 years ago

In 48952:

Tests: Add a polyfill for assertEqualsWithDelta() to WP_UnitTestCase and use it where appropriate.

assertEqualsWithDelta() was added in PHPUnit 7.5, while WordPress still supports PHPUnit 5.4.x as the minimum version.

See #38266.

#21 @SergeyBiryukov
4 years ago

In 48953:

Tests: Remove the polyfill for assertNotFalse().

assertNotFalse() is available in PHPUnit since version 4.0.

The polyfill was introduced back when WordPress still supported PHP 5.2 and PHPUnit 3.6.x, and is now redundant.

Follow-up to [39919], [45058], [47880].

See #38266.

#22 @SergeyBiryukov
4 years ago

In 48954:

Tests: Replace a few instances of assertNotEquals() with assertNotFalse().

See #38266.

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


4 years ago

#24 @SergeyBiryukov
4 years ago

In 48974:

Tests: Correct assertion in Tests_DB::test_prepare_incorrect_arg_count().

On failure, wpdb::prepare() returns either an empty string or null, not false.

The test only passed accidentally due to assertEquals() not performing a strict type check.

Follow-up to [41662].

See #38266.

#25 @SergeyBiryukov
3 years ago

In 49051:

Tests: Correct assertion in WP_Test_REST_Comments_Controller::check_comment_data().

author_avatar_urls should be present in the comment data array keys, not values.

The test only passed accidentally due to assertContains() not performing a strict type check.

See #38266, #50913.

#26 @SergeyBiryukov
3 years ago

In 49126:

Tests: Use assertSame() in test_edit_user_blank_password(), for consistency with other assertions.

Follow-up to [49118].

See #42766, #38266.

#27 @SergeyBiryukov
3 years ago

In 49547:

Tests: Use assertSame() in some newly introduced tests.

This ensures that not only the return values match the expected results, but also that their type is the same.

Going forward, stricter type checking by using assertSame() should generally be preferred to assertEquals() where appropriate, to make the tests more reliable.

Follow-up to [48937], [48939], [48940], [48944].

See #38266.

#28 @SergeyBiryukov
3 years ago

  • Type changed from defect (bug) to task (blessed)

#29 @hellofromTonya
3 years ago

  • Milestone changed from 5.6 to 5.7

Punting this ticket to 5.7 as today is 5.6 RC2 (though this one could easily be a close and then reopen new for 5.7).

If any maintainer or committer feels this can be resolved in time, or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

#30 @johnbillion
3 years ago

In 49947:

Taxonomy: Correct and clarify documentation for the return types of term query functions.

See #51800, #38266

#31 in reply to: ↑ 19 @SergeyBiryukov
3 years ago

Replying to SergeyBiryukov:

In other cases, this points to minor bugs in core, e.g. using ceil() for values that are documented as integers, but without explicitly casting to int, results in them being of type float instead. This affects some properties like max_num_pages, max_pages, total_pages in various classes, or functions like get_comment_pages_count(), wp_embed_defaults(), get_oembed_response_data().

Related: #46346

#32 follow-up: @johnbillion
3 years ago

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

This feels like an ongoing ticket. There's more than 700 instances of assertEquals in use and quite a few of those are still checking falsey values.

Closing as fixed for 5.7, I'll open a follow-up for 5.8.

#34 in reply to: ↑ 32 @SergeyBiryukov
3 years ago

Replying to johnbillion:

There's more than 700 instances of assertEquals in use and quite a few of those are still checking falsey values.

If we ignore assertEqualsWithDelta() and search specifically for assertEquals(), there are currently 653 matches in 128 files. This has gone up a bit since WordPress 5.6: comment:19 (572 matches in 121 files), so apparently some newly introduced tests use assertEquals() instead of assertSame(). Some of those were corrected in [49126] and [49547], but more were added in 5.7.

Let's continue on #52482 though :)

#35 @SergeyBiryukov
3 years ago

In 50283:

Tests: Replace most instances of assertEquals() in phpunit/includes/ with assertSame().

Follow-up to [48937], [48939], [48940], [48944].

Props johnbillion, jrf, SergeyBiryukov.
See #38266, #52482.

#36 @SergeyBiryukov
3 years ago

In 50284:

Tests: Use assertSame() in some newly introduced tests.

This ensures that not only the return values match the expected results, but also that their type is the same.

Going forward, stricter type checking by using assertSame() should generally be preferred to assertEquals() where appropriate, to make the tests more reliable.

Follow-up to [49904], [49925], [49992], [50012], [50013], [50065], [50075], [50131], [50150], [50157].

See #38266, #52482.

Note: See TracTickets for help on using tickets.