Tests: Reduce usage of assertEquals for 5.9

[desrosj]

Follow-up to:

• #52482 for 5.8
• #38266 for 5.7

The assertEquals() test method does not check that the types of the expected and actual values match. This can hide subtle bugs especially when the values are falsey.

Tasks:

• Switch to using assertSame() when the type of the value in the assertion is important
• Replace overall usage of assertEquals() with type-strict assertion methods, with the aim of potentially removing its usage altogether

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.

[costdev]

Overview

I've taken a closer look at the test suite and usage of assertEquals(). My thoughts on the stages to take are below.

Stage 1:

'Clean' replacements.

PR 1768 replaces some instances of assertEquals() with assertSame(). More specifically, it makes replacements that appear to be 'clean' replacements, requiring no adjustment of Tests or Core code. All PHPUnit tests pass.

Stage 2:

I have looked at the cases where the returned value is float rather than int.

As @SergeyBiryukov noted here:

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

A related ticket further down has a lengthy discussion about ways to resolve this.

• I've tested one of the proposed solutions, a simple cast to int using (int) before the calls to ceil(), followed by changing assertEquals() to assertSame() for relevant tests. All PHPUnit tests pass.
• With coverage incomplete this could, theoretically, be a breaking change. However, we are yet to come across a real-world example of this, or even an example scenario where these methods should return a float.

Stage 3

I have looked at the cases where the returned value is string rather than int.

After investigation, the actual values are a numeric string for compatibility reasons and are documented as such. For this reason, the expected value should be a numeric string, rather than changing the code in src and breaking compatibility.

Stage 4

The returned value is array and assertEquals() is being used instead of assertSameSets(). It should be noted that an array containing an object will not return the same signature, thus cannot be tested using assertSameSets() or assertSameSetsWithIndex().

Stage 5

The returned value is always bool rather than int. In these cases, the expected value should be changed to a bool.

This can be done directly e.g. assertTrue() / assertFalse() / true|false in data providers.

This one requires particularly close attention to the methods being tested, their possible return values and the accuracy of the current tests.

Stage 6

The returned value is float rather than int. This requires investigation to determine why the value is float and whether this is correct, should be changed in src, or must remain float for compatibility reasons, thus the expected value in the tests should be changed to float.

Stage 7

N.B. This may go outside the scope of this ticket.

The returned value is array and assertEqualSets() or assertEqualSetsWithIndex() is being used instead of assertSameSets() and assertSameSetsWithIndex() respectively. It should be noted that an array containing an object will not return the same signature, thus cannot be tested using assertSameSets() or assertSameSetsWithIndex().

[costdev]

Report

Stage 0

Changes: None
Results for assertEquals(): 528 results in 118 files. ( -0 )

Stage 1

Changes:

• PR 1768 - Replace assertEquals() with assertSame() where tests pass with no other code requiring changes.

Results for assertEquals(): 519 results in 118 files. ( -9 )

Status: The PR is ready for review.

Stage 2

Changes:

• All of the above (for calculations, not for the patch)
• Cast ceil() to int throughout non-third party .php files in src.
• Change assertEquals() to assertSame() for relevant tests.

Results for assertEquals(): 493 results in 115 files. ( -26 )

Notes:

• Remaining tests that compare an int to a float using assertEquals() are not affected by the ceil() change. As a result, these have been left until Stage 6 for investigation.

Status:

• Pending feedback on the approach.
• The patch is prepared and with some self-review, will be ready for PR.

Stage 3

Changes:

• All of the above (for calculations, not for the patch)
• For most assertions that compare a raw int to a string response, wrap the expected value in ''.
• For some in tests/phpunit/tests/image/meta.php, these are asserting camera shutter speed in EXIF data. These have been casted to string using, for example, (string) ( 1 / 40 ).

Results for assertEquals(): 402 results in 112 files. ( -91 )

Status:

• Pending feedback on the approach.
• The patch is prepared and with some self-review, will be ready for PR.

[SergeyBiryukov]

In 52266:

Tests: Replace assertEquals() with assertSame() in block template 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(), assertSameSets(), or assertSameSetsWithIndex() should generally be preferred, to make the tests more reliable.

Follow-up to [51003], [51079], [52062], [52265].

See #53364, #53363, #54335.

[costdev]

Update

​The PR currently reduces assertEquals() usage from ~558 to ~191 through various changes to assertions. There are currently no changes in source.

Regarding the remaining ~191:
As we know, many of them are comparing objects, so using assertEquals() is fine. But, it's a pain to review those every release to make sure that we're not missing anything. It also gives us a false perception of the estimated number of unacceptable uses of assertEquals() for us to deal with for that release.

What about two additional assertions that are just aliases of assertEquals()?

// For two objects.
// We can't strictly compare objects in most cases anyway,
// so this name should be safe and the implied comparison
// is accurate in all but object id.
public function assertSameObjects( $expected, $actual, $message = '' ) {
        $this->assertEquals( $expected, $actual, $message );
}

// For arrays containing objects.
public function assertSameSetsWithObjects( $expected, $actual, $message = '' ) {
        $this->assertEquals( $expected, $actual, $message );
}

The intended benefit is a clearer picture of possible loose comparison with assertEquals() by excluding known acceptable usage. There's also a sort of maybe benefit that by offering the assertion, it reinforces the idea of stricter testing. This would reduce the remaining ~191 by quite a bit and minimize the work involved with each release.

For the remainder, these are largely due to float being compared to int. For nearly all of these, this is an issue with ceil(). It has been suggested before that we can cast the result of ceil() to int to remedy this when the value is documented as an int.

So, a call for opinions:

1. Should we add the two aliases to make our lives easier going forward?
2. Should we cast ceil() to int where appropriate to help us implement stricter comparison?

[SergeyBiryukov]

Replying to costdev:

What about two additional assertions that are just aliases of assertEquals()?

// For two objects.
// We can't strictly compare objects in most cases anyway,
// so this name should be safe and the implied comparison
// is accurate in all but object id.
public function assertSameObjects( $expected, $actual, $message = '' ) {
        $this->assertEquals( $expected, $actual, $message );
}

// For arrays containing objects.
public function assertSameSetsWithObjects( $expected, $actual, $message = '' ) {
        $this->assertEquals( $expected, $actual, $message );
}

This sounds like a great idea to me!

So, a call for opinions:

1. Should we add the two aliases to make our lives easier going forward?
2. Should we cast ceil() to int where appropriate to help us implement stricter comparison?

I'd say yes to both.

[hellofromTonya]

With 5.9 RC1 tomorrow, closing this ticket as fixed. Ongoing work will continue in #54726 during the 6.0 cycle.

Thank you everyone for your contributions!!