Make WordPress Core

Opened 12 months ago

Closed 8 months ago

Last modified 7 months ago

#52482 closed task (blessed) (fixed)

Tests: Reduce usage of assertEquals() for 5.8

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

#1 @johnbillion
12 months ago

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

#2 @johnbillion
12 months ago

  • Description modified (diff)
  • Keywords php8 added

#3 @jrf
12 months 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
12 months 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 12 months ago by SergeyBiryukov (previous) (diff)

#5 @jrf
12 months 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
12 months 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
12 months 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
12 months 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.

#9 @SergeyBiryukov
8 months ago

  • Owner set to SergeyBiryukov
  • Status changed from new to accepted

#10 @Simivar
8 months ago

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?

#11 @SergeyBiryukov
8 months ago

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.

#12 @desrosj
8 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

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.

#13 @SergeyBiryukov
8 months ago

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.

#14 @SergeyBiryukov
7 months ago

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.

#15 @SergeyBiryukov
7 months ago

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.

Note: See TracTickets for help on using tickets.