WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 13 days ago

#38266 accepted defect (bug)

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

Reported by: johnbillion Owned by: SergeyBiryukov
Milestone: 5.6 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 (26)

#1 @johnbillion
4 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
17 months ago

  • Keywords needs-unit-tests added; ongoing removed

#3 @johnbillion
16 months 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.


5 months ago

#5 @jrf
2 months 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
2 months 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
7 weeks ago

  • Keywords php8 added

#8 @SergeyBiryukov
7 weeks 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
7 weeks ago

@SergeyBiryukov My HERO!!! <3

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


7 weeks ago

#11 @SergeyBiryukov
7 weeks 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
7 weeks 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.


7 weeks ago

#14 @SergeyBiryukov
7 weeks 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
7 weeks 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
7 weeks 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
7 weeks 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
7 weeks ago

In 48950:

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

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

See #38266.

#19 @SergeyBiryukov
7 weeks 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 7 weeks ago by SergeyBiryukov (previous) (diff)

#20 @SergeyBiryukov
7 weeks 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
7 weeks 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
7 weeks 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.


6 weeks ago

#24 @SergeyBiryukov
6 weeks 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
4 weeks 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
13 days ago

In 49126:

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

Follow-up to [49118].

See #42766, #38266.

Note: See TracTickets for help on using tickets.