Make WordPress Core

Opened 2 years ago

Closed 20 months ago

#55654 closed task (blessed) (fixed)

Tests: Reduce usage of assertEquals for 6.1

Reported by: hellofromtonya's profile hellofromTonya Owned by: desrosj's profile desrosj
Milestone: 6.1 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch has-unit-tests
Focuses: 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 backporting tests, changes should also be made upstream in the Gutenberg repository.

Change History (12)

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


2 years 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/55654

#2 @desrosj
20 months ago

  • Keywords needs-refresh added

@costdev could you refresh the pull request? Looks like there are merge conflicts.

#3 @costdev
20 months ago

  • Keywords needs-refresh removed

@desrosj Done.

  1. Merge conflicts have been resolved.
  2. New instances of assertEquals() have been replaced with more appropriate assertions.
  3. assertArrayEqualsWithObject(), proposed in the PR, has been removed, as assertEqualSets() performs more testing and has the same result.

Before the PR
568 instances of assertEquals().

With the PR (as of this comment)
34 instances of assertEquals().

No source changes.

Last edited 20 months ago by costdev (previous) (diff)

#4 @desrosj
20 months ago

  • Owner set to desrosj
  • Status changed from new to reviewing

#6 @desrosj
20 months ago

In 54402:

Tests: Replace some occurrences of assertEquals() with assertSame().

This ensures that not only the return values match the expected results, but also that their type is the same.

Props costdev, desrosj.
See #55654.

#8 follow-up: @desrosj
20 months ago

@costdev Thanks for that PR and all the work so far.

This one is a bear, so I started with tests that did not involve type coercion for the expected values to get the ball rolling. We'll have to work through this in chunks, and possibly push parts for continuing in 6.2.

Some of these just require a deeper look to confirm the intended type is being checked for.

#9 in reply to: ↑ 8 @SergeyBiryukov
20 months ago

Thanks for the commit! [54402] looks great to me.

Replying to desrosj:

Some of these just require a deeper look to confirm the intended type is being checked for.

Right, that is the reason I was careful with this in earlier iterations. As noted in comment:19:ticket:38266:

  • In some cases, the tests should be adjusted to use the correct data type.
  • In other cases, this points to minor bugs in core, e.g. using ceil() for values that are documented as integers, but without explicitly casting to int, results in them being of type float instead. This affects some properties like max_num_pages, max_pages, total_pages in various classes, or functions like get_comment_pages_count(), wp_embed_defaults(), get_oembed_response_data().

#10 @costdev
20 months ago

Thanks @desrosj!

Following @SergeyBiryukov's comment above regarding ceil(), dropping a related comment I posted during the 5.9 cycle:

  • I've tested one of the proposed solutions, a simple cast to int using (int) before the calls to ceil(), followed by changing assertEquals() to assertSame() for relevant tests. All PHPUnit tests pass.
  • With coverage incomplete this could, theoretically, be a breaking change. However, we are yet to come across a real-world example of this, or even an example scenario where these methods should return a float.

#11 @SergeyBiryukov
20 months ago

In 54446:

Tests: Use assertSame() in some WP_Theme_JSON tests.

This ensures that not only the return values match the expected results, but also that their type is the same.

Previously committed in [54402], these instances appear to be accidentally reverted to assertEquals() in [54443].

Follow-up to [52275], [54162], [54402], [54443].

See #55654.

#12 @desrosj
20 months ago

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

Going to close this one out as RC1 is due out any moment. #56800 has been created to continue this in 6.2.

Note: See TracTickets for help on using tickets.