Make WordPress Core

Opened 6 months ago

Closed 3 months ago

Last modified 7 weeks ago

#64324 closed task (blessed) (fixed)

Tests: Reduce usage of assertEquals for 7.0

Reported by: sergeybiryukov's profile SergeyBiryukov Owned by:
Milestone: 7.0 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch has-unit-tests
Focuses: coding-standards Cc:

Description

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

To help ease the effort of merging tests, changes should also be made upstream in the Gutenberg repository.

Change History (18)

This ticket was mentioned in PR #1768 on WordPress/wordpress-develop by @costdev.


6 months ago
#1

  • Keywords has-patch has-unit-tests added

This PR replaces assertEquals() with more appropriate, stricter assertions where possible and without making any changes to source.

For easier reviewing, commits have been separated based on the replacement assertion and any additional changes required to implement the stricter assertion.

Trac ticket: https://core.trac.wordpress.org/ticket/64324
Trac ticket: https://core.trac.wordpress.org/ticket/63169
Trac ticket: https://core.trac.wordpress.org/ticket/62278
Trac ticket: https://core.trac.wordpress.org/ticket/61573
Trac ticket: https://core.trac.wordpress.org/ticket/60706
Trac ticket: https://core.trac.wordpress.org/ticket/59655
Trac ticket: https://core.trac.wordpress.org/ticket/55654
Trac ticket: https://core.trac.wordpress.org/ticket/54726
Track ticket: https://core.trac.wordpress.org/ticket/63169

This ticket was mentioned in PR #4132 on WordPress/wordpress-develop by @Rahmohn.


6 months ago
#2

This PR updates the test test_wp_count_attachments_should_cache_the_result to check the properties of the object returned instead of checking the object with assertEquals. That way, we guarantee the expected properties of the object.

Trac ticket:
https://core.trac.wordpress.org/ticket/64324
https://core.trac.wordpress.org/ticket/63169

#3 @SergeyBiryukov
6 months ago

In 61364:

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

See #64324.

#4 @SergeyBiryukov
6 months ago

In 61367:

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

See #64324.

#5 @SergeyBiryukov
6 months ago

In 61370:

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 [60775], [61131].

See #64324.

#6 @SergeyBiryukov
6 months ago

In 61373:

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 [61032], [61045].

See #64324.

#7 @SergeyBiryukov
6 months ago

In 61413:

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 [60490], [60524].

See #64324.

#8 @SergeyBiryukov
5 months ago

In 61417:

Tests: Use assertSame() in wp_count_posts() 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 [1323/tests], [25554], [27081], [60788].

Props costdev, SergeyBiryukov.
See #64324.

#9 @SergeyBiryukov
5 months ago

In 61420:

Tests: Use assertSame() in populate_network() 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 [60954].

See #64324.

#10 @desrosj
3 months ago

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

Opened #64895 for the 7.1 release cycle. With 7.0 RC1 due out today, going to close this out. This can be reopened or referenced if there are more changes needed related to this during 7.0.

This ticket was mentioned in PR #11602 on WordPress/wordpress-develop by @sagardeshmukh.


8 weeks ago
#11

Replaces loose assertEquals() with assertSame() for scalar integer ID comparisons in the get_adjacent_post() and get_boundary_post() tests.

Object comparisons against full WP_Post instances are intentionally kept as assertEquals(), since assertSame() uses strict === comparison which requires the same PHP object instance, that would fail for objects fetched via separate database calls.

See https://core.trac.wordpress.org/ticket/64324

This ticket was mentioned in PR #11605 on WordPress/wordpress-develop by @sagardeshmukh.


8 weeks ago
#12

Migrates assertEquals() to assertSame() in tests/phpunit/tests/ai-client/wpAiClientPromptBuilder.php.

All 78 assertEquals() calls compare scalar values (strings, ints, floats, or string arrays), so strict === comparison is appropriate. Two assertions comparing timeout values were updated to use float literals (30.0, 45.0) to match the actual return type of getTimeout().

172 tests, 439 assertions — all passing.

See https://core.trac.wordpress.org/ticket/64324

#13 @SergeyBiryukov
7 weeks ago

In 62247:

Tests: Use assertSame() in get_adjacent_post() 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 [60733], [61066].

Props sagardeshmukh.
See #64324.

@SergeyBiryukov commented on PR #11602:


7 weeks ago
#14

Thanks for the PR! Merged in r62247.

#15 @SergeyBiryukov
7 weeks ago

In 62248:

Tests: Use assertSame() in WP_AI_Client_Prompt_Builder 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 [61700].

Props sagardeshmukh.
See #64324.

@SergeyBiryukov commented on PR #11605:


7 weeks ago
#16

Thanks for the PR! Merged in r62248.

#17 @peterwilsoncc
7 weeks ago

I'm going to merge r62248 to the 7.0 branch as it's required to avoid conflicts in the backport for r62255.

#18 @peterwilsoncc
7 weeks ago

In 62265:

Tests: Use assertSame() in WP_AI_Client_Prompt_Builder 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 [61700].

Reviewed by peterwilsoncc.
Merges r62248 to the 7.0 branch.

Props sagardeshmukh, SergeyBiryukov.
See #64324.

Note: See TracTickets for help on using tickets.