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

[johnbillion]

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.

[johnbillion]

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.

[johnbillion]

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.

[jrf]

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.

[SergeyBiryukov]

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.

[SergeyBiryukov]

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.

[jrf]

@SergeyBiryukov My HERO!!! <3

[SergeyBiryukov]

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.

[SergeyBiryukov]

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.

[SergeyBiryukov]

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.

[SergeyBiryukov]

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.

[SergeyBiryukov]

• 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.

[SergeyBiryukov]

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.

[SergeyBiryukov]

In 48950:

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

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

See #38266.

[SergeyBiryukov]

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().

[SergeyBiryukov]

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.

[SergeyBiryukov]

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.

[SergeyBiryukov]

In 48954:

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

See #38266.

[SergeyBiryukov]

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.

[SergeyBiryukov]

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.

[SergeyBiryukov]

In 49126:

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

Follow-up to [49118].

See #42766, #38266.

[SergeyBiryukov]

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.

[hellofromTonya]

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.

[johnbillion]

In 49947:

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

See #51800, #38266

[SergeyBiryukov]

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

[johnbillion]

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.

[johnbillion]

#52482 for 5.8.

[SergeyBiryukov]

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 :)

[SergeyBiryukov]

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.

[SergeyBiryukov]

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.