Tests: Reduce usage of assertEquals() for 5.8

[johnbillion]

Follow-up to:

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

[jrf]

Replace overall usage of assertEquals() with type-strict assertion methods, with the aim of potentially removing its usage altogether

One note about this: when comparing whether two objects of the same type are "equal", using assertEquals() is still warranted. assertSame() would fail for objects of the same type with the same property values, as the ID of the object will, in most cases, not be the same.

For a limited number of objects (ValueObjects), it may be possible to switch the assertEquals() out in favour of using ​`assertObjectEquals()` in the future, but that would either require PHPUnit 9.4+ or a polyfill until that time.

[SergeyBiryukov]

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:ticket:38266 (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.

Would it be appropriate to add an automated check for this, or at least recommend using assertSame() in the core contributor handbook? I've updated the examples in ​Writing PHPUnit Tests to use assertSame(), but perhaps we could also explicitly state somewhere that it should be preferred to assertEquals() where possible.

Replying to jrf:

One note about this: when comparing whether two objects of the same type are "equal", using assertEquals() is still warranted. assertSame() would fail for objects of the same type with the same property values, as the ID of the object will, in most cases, not be the same.

Indeed, also previously noted in comment:19:ticket:38266. Thanks for pointing that out!

[jrf]

I've updated the examples in Writing PHPUnit Tests to use assertSame()

👍

Would it be appropriate to add an automated check for this...

Those are in the making (PHPUnitQA PHPCS standard), but it may still quite be a while before there is a ready-to-use release.
In the mean time, for at least some parts of this, feel free to ping me either with ideas for scans to add or to check if I have a scan available & for me to post the results somewhere.

or at least recommend using assertSame() in the core contributor handbook

Sounds like a good idea.

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

[SergeyBiryukov]

Replying to SergeyBiryukov:

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:ticket:38266 (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.

[50283] and [50284] further reduce the number of assertEquals() instances to 533 matches in 121 files.

[Simivar]

In PHP-CS-Fixer there's already php_unit_strict rule. Maybe it would be a good idea to use that tool as well so there's no need to develop additional custom rules or wait for it to be developed and use battle-tested ones instead?

[SergeyBiryukov]

In 51079:

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 [50380], [50959], [50960], [50973], [50993], [51003], [51051], [51054].

See #52482.

[desrosj]

I've opened #53364 to continue this during the 5.9 cycle. If there are more commits during the 5.8 beta cycle (which are allowed for test only changes), this ticket can still be referenced.

[SergeyBiryukov]

In 51137:

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

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

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

Follow-up to [48939], [49925], [50157], [50959], [50960], [50995], [51079].

See #52625, #52482.

[SergeyBiryukov]

In 51220:

Tests: Replace assertEquals() with assertSameSets() in text widget tests.

This ensures that not only the array values being compared are equal, 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 [40631], [41132], [48939], [51137].

See #52482, #52625.

[SergeyBiryukov]

In 51225:

Tests: Use assertSame() in _wp_to_kebab_case() 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 [51079], [51198].

See #52482, #52625, #53397.