WordPress.org

Make WordPress Core

Opened 2 weeks ago

Last modified 2 weeks ago

#52482 new task (blessed)

Tests: Reduce usage of assertEquals() for 5.8

Reported by: johnbillion Owned by:
Milestone: 5.8 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: needs-unit-tests php8
Focuses: Cc:

Description (last modified by johnbillion)

Follow-up to:

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

Change History (8)

#1 @johnbillion
2 weeks ago

  • Summary changed from Tests: Reduce usage of assertSame() for 5.8 to Tests: Reduce usage of assertEquals() for 5.8

#2 @johnbillion
2 weeks ago

  • Description modified (diff)
  • Keywords php8 added

#3 @jrf
2 weeks ago

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.

#4 follow-up: @SergeyBiryukov
2 weeks ago

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!

Last edited 2 weeks ago by SergeyBiryukov (previous) (diff)

#5 @jrf
2 weeks ago

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.

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

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

#8 in reply to: ↑ 4 @SergeyBiryukov
2 weeks ago

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.

Note: See TracTickets for help on using tickets.