WordPress.org

Make WordPress Core

Opened 5 months ago

Closed 5 months ago

Last modified 5 months ago

#40270 closed defect (bug) (fixed)

Incorrect error assertions in some REST API tests

Reported by: dlh Owned by: jnylen0
Milestone: 4.8 Priority: normal
Severity: normal Version:
Component: REST API Keywords:
Focuses: Cc:

Description (last modified by dlh)

The test_delete_item() and test_delete_item_skip_trash() methods in WP_Test_REST_Posts_Controller contain the assertion $this->assertNotInstanceOf( 'WP_Error', $response );.

If I'm reading correctly, these assertions will always pass because they check a WP_REST_Response object. Instead, the assertions should check $response->as_error(), as happens in WP_Test_REST_Users_Controller.

The attached patch updates these assertions, as well as some similar assertions in WP_Test_REST_Post_Type_Controller_Testcase.

In the post type assertions, it looks as though the $response can be a WP_REST_Response, so it passes assertNotInstanceOf( 'WP_Error' ) but is still capable of returning a WP_Error via as_error().

The rest of the assertions in those post type methods might make the as_error() check unnecessary, though.

Attachments (2)

40270.diff (2.7 KB) - added by dlh 5 months ago.
40270.2.diff (904 bytes) - added by dlh 5 months ago.

Download all attachments as: .zip

Change History (7)

@dlh
5 months ago

#1 @dlh
5 months ago

  • Description modified (diff)

#2 @jnylen0
5 months ago

Good catch @dlh.

This is the preferred way of verifying that a response is not an error:

$this->assertEquals( 200, $response->get_status() );

(or 201 as needed).

These assertions are already present for the cases you noted, so let's just remove the two assertNotInstanceOf lines. I don't see any other such lines in the REST API tests.

@dlh
5 months ago

#3 @dlh
5 months ago

Thanks for those details, @jnylen0. 40270.2.diff removes the two lines.

#4 @jnylen0
5 months ago

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

In 40350:

Tests: Remove a couple of invalid error assertions.

A couple of REST API tests had an assertion assertNotInstanceOf( 'WP_Error', $response ); which will never be true.

Since these assertions are invalid, and also made redundant by the response status check, we can just remove them.

Props dlh.
Fixes #40270.

#5 @swissspidy
5 months ago

  • Milestone changed from Awaiting Review to 4.8
Note: See TracTickets for help on using tickets.