#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 )
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
Change History (15)
#1
@
4 years ago
- Summary changed from Tests: Reduce usage of assertSame() for 5.8 to Tests: Reduce usage of assertEquals() for 5.8
#4
follow-up:
↓ 8
@
4 years 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!
#5
@
4 years 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.
#8
in reply to:
↑ 4
@
4 years ago
Replying to SergeyBiryukov:
If we ignore
assertEqualsWithDelta()
and search specifically forassertEquals()
, 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 useassertEquals()
instead ofassertSame()
. 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.
#10
@
4 years 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?
#12
@
3 years 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
@
3 years 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].
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.