Make WordPress Core

Opened 8 months ago

Closed 3 months ago

Last modified 3 months ago

#56800 closed defect (bug) (fixed)

Tests: Reduce usage of assertEquals for 6.2

Reported by: desrosj's profile desrosj Owned by: hellofromtonya's profile hellofromTonya
Milestone: 6.2 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch has-unit-tests commit
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 (17)

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


3 months ago
#1

  • Keywords has-patch has-unit-tests added

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/56800

This ticket was mentioned in Slack in #core by costdev. View the logs.


3 months ago

#3 @hellofromTonya
3 months ago

Opened #57855 for the ongoing work into the 6.3 cycle.

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


3 months ago
#4

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

Trac ticket: https://core.trac.wordpress.org/ticket/56800

#5 @costdev
3 months ago

I've submitted PR 4166 to replace some occurrences of assertEquals() with assertSame(). These are switcharoo changes that don't involve type-coercion or any other change.

It seems that only 7 new instances of assertEquals() have been introduced since last cycle (improvement!), with an additional one that was missed during the last attempts to reduce assertEquals() usage.

Pinging @hellofromTonya for review of PR 4166.


Note that there are 462 uses of $this->assertEquals() remaining in the test suite. Many of these are testing objects, and are therefore fine. However, I think we should pursue the intention to add two new assertion conventions in 6.3:

  • assertSimilarObject() - A new assertion that acts as a wrapper for assertIsObject() and assertEquals() when comparing objects.
  • assertArrayEquals() - An existing assertion for use when comparing arrays containing at least one object.

Why?
Every cycle, we have to sift through these types of assertions to determine whether their usage is fine. This is a huge time sink, and as shown by the data above, meant that putting PR 4166 together required reviewing 470 instances of assertEquals(), many of which were testing objects/arrays with objects. The above assertions would remove some of this significant overhead, as well as add some context to the assertions.

When?
I can submit a PR for the new assertSimilarObject() assertion as soon as we branch for 6.3. There was support for using these assertions in previous cycles, but we never got around to it.


Other remaining instances include:

  • $expected is not the same type as $actual (usually for int or float when $actual is string).
    • There is no need for these to be assertEquals() when a simple type change will make the test accurate.
  • $actual is returning a float when it's documented to return int.
    • This is essentially an issue with some ceil() usage in Core that needs appropriately rectified. Note: (int) casts alone are not always appropriate.
  • The assertion should be changed to one that isn't assertSame(). These include:
    • assertTrue(), assertFalse(), assertNull(), assertSameSetsWithIndex(), assertEqualSetsWithIndex()* and assertEqualSets()*.

* Requires further review if the two new assertions are implemented.

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

#6 @hellofromTonya
3 months ago

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

Thank you @costdev. Reviewing now.

#7 @hellofromTonya
3 months ago

  • Keywords commit added

PR 4166 is ready for commit. Prepping now.

#8 @hellofromTonya
3 months ago

In 55465:

Build/Test Tooling: Use assertSame() in WP_Date_Query tests.

Change from assertEquals() to assertSame(). Why? To ensure both the return value and data type match the expected results.

Follow-up to [54530].

Props costdev, peterwilsoncc, mukesh27, ankitmaru.
See #56800.

#9 @hellofromTonya
3 months ago

In 55466:

Build/Test Tooling: Use assertSame() in Tests_Comment::test_update_comment_from_privileged_user_by_privileged_user().

Change from assertEquals() to assertSame(). Why? To ensure both the return value and data type match the expected results.

Follow-up to [c].

Props costdev, peterwilsoncc, mukesh27, ankitmaru.
See #56800.

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


3 months ago
#10

Follow-up to PR #4166.

Changes from assertEquals() to assertSame(). Why? To ensure both the return value and data type match the expected results.

Changes the expected height and width from string to integer data types. Why integer? getimagesize() (within wp_getimagesize()) will return an integer for both height and weight.

PR is a confidence check before committing.

Trac ticket: https://core.trac.wordpress.org/ticket/56800

#11 @hellofromTonya
3 months ago

In 55467:

Tests: Improve Tests_Media::test_wp_generate_attachment_metadata_doesnt_generate_sizes_for_150_square_image().

Changes:

  • from assertEquals() to assertSame(). Why? To ensure both the return value and data type match the expected results.
  • the expected height and width from string to integer data types. Why integer? getimagesize() (within wp_getimagesize()) will return an integer for both height and weight.
  • adds the ticket annotation.
  • adds assertion failure messages. Why? To denote which assertion failed, which aids in debugging efforts.

Follow-up to [55278].

Props costdev, peterwilsoncc, mukesh27, ankitmaru, hellofromTonya.
See #56800, #57370.

#13 @hellofromTonya
3 months ago

In 55468:

Tests: Use assertSame() in Tests_Theme_wpThemeJson.

Change from assertEquals() to assertSame(). Why? To ensure both the return value and data type match the expected results.

Follow-up to [55216].

Props costdev, peterwilsoncc, mukesh27, ankitmaru.
See #56800, #57621.

@hellofromTonya commented on PR #4166:


3 months ago
#14

Closing as changes were committed.

This ticket was mentioned in Slack in #core by costdev. View the logs.


3 months ago

#16 @hellofromTonya
3 months ago

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

#17 @hellofromTonya
3 months ago

Closing for 6.2. If more commits are needed and can happen before RC1 (in 2 days), then please reopen.

Note: See TracTickets for help on using tickets.