WordPress.org

Make WordPress Core

Opened 2 weeks ago

Closed 11 days ago

#42124 closed defect (bug) (fixed)

Unit test errors with PHPUnit 6.4.0

Reported by: joemcgill Owned by: SergeyBiryukov
Milestone: 4.9 Priority: high
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch 2nd-opinion
Focuses: Cc:

Description

Change in PHPUnit 6.4.0, resulted in our tests to start failing. Specifically,

1) Tests_Image_Functions::test_load_directory
TypeError: get_resource_type() expects parameter 1 to be resource, string given
/home/travis/build/WordPress/wordpress-develop/tests/phpunit/tests/image/functions.php:269

This test uses assertNotInternalType() to ensure wp_load_image() and wp_get_image_editor() wouldn't load a link to a resource (see #17814). Under the hood, PHPUnit uses get_resource_type() in this assertion, which will produce an error that was previously not surfaced.

Attachments (1)

42124.diff (769 bytes) - added by joemcgill 2 weeks ago.

Download all attachments as: .zip

Change History (7)

@joemcgill
2 weeks ago

#1 @joemcgill
2 weeks ago

  • Keywords has-patch 2nd-opinion added

42124.diff Updates the failing test so we check for the expected variable type, rather than testing that we're not returning a resource type.

#2 @johnjamesjacoby
2 weeks ago

This patch seems right.

get_resource_type() accepts a PHP Resource variable type, so the resource string would not be sufficient in assertNotInternalType(); it would need to be an actual resource type variable. If this was working before PHPUnit 6.4.0, it probably should not have been.

WordPress uses Resource variables in quite a few places (where server & database connections are used) but also generally shields us from needing to do very much with them until we hit API bedrock. @joemcgill you're probably familiar with them with your recent trip down MimeType boulevard.

In these test cases, these variables are not resources, and your patch uses the correct assertions for the types that they are.

Last edited 2 weeks ago by johnjamesjacoby (previous) (diff)

#3 @SergeyBiryukov
2 weeks ago

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

In 41784:

Build/Test Tools: In Tests_Image_Functions::test_load_directory(), check the expected internal type instead of using assertNotInternalType(), which causes an error with PHPUnit 6.4.0.

Props joemcgill.
Fixes #42124.

#4 @joemcgill
2 weeks ago

For future reference, I've reported this issue upstream to PHPUnit here:
https://github.com/sebastianbergmann/phpunit/issues/2792

#5 @joemcgill
2 weeks ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

This will be fixed in PHPUnit 6.4.1. (see commit). We may want to consider reverting [41784] as the previous approach was more accurate. Alternately, we could use assertNotSame( 'resource', gettype( $editor ) ) in both assertions.

#6 @pento
11 days ago

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

In 41841:

Build/Test Tools: Revert [41784].

[41784] was a workaround for a bug in PHPUnit 6.4. PHPUnit 6.4.1 has since been released, which includes a fix for this.

Fixes #42124.

Note: See TracTickets for help on using tickets.