Tests_Image_Intermediate_Size::test_get_intermediate_sizes_by_array_zero_width fails when the random value is 201

[jorbin]

This is an intermittent test failure. I'm not sure if it fails due to 201 finding an underlying bug or there being a bug in the test code.

[jorbin]

The above patch uses a data provider instead of random to help demonstrate this failure.

[boonebgorges]

I spent a few minutes with this. I don't understand the size-selection logic well enough to fully grok what's happening. But it's roughly this:

• The test in question is meant to demonstrate that image_get_intermediate_size() will skip the 'false-height' image size.
• In the case where 'false-height' is defined with a height of 201, 'false-height' is not skipped.
• The original image being cropped, waffles.jpg, is 600x400. This is close enough that attempting to crop it into a height of 201 will maintain a width of 300.
• 300 is within the 1px margin of error that image_get_intermediate_size() uses to determine whether a potential size has a different aspect ratio from an available image size.
• So 300x201 ends up being a viable image size, not a "false" height image size.

See https://core.trac.wordpress.org/ticket/17626#comment:32 and the following few comments.

I'm not sure whether this is a bug in the function. At a glance, I'd say that if we are OK with the general principle of allowing a 1px margin of error for aspect ratio equivalence, then the function is probably performing as expected. That makes it a bug in the test: the 300x201 aspect ratio does not match "a single dimention" [sic], it in fact matches both dimensions.

Using random number/string generators in unit tests is a pretty bad idea. In some cases, it may show us edge case bugs, but that's not worth the trade-off of having tests that act differently each time you run them. This test probably ought to be rewritten to be hardcoded with a number that is just large enough to cause the aspect ratio check to fail: 202.

[boonebgorges]

Very quick follow-up: I think that the failure of this test is causing Tests_Media::test_add_image_size() to fail. (Hooray for globals!) The test suite ought to remove custom image sizes during tearDown() or setUp().

[boonebgorges]

On the basis of my analysis above, I'm going to split the current test into two: one that does what the original test was supposed to do (with a value of 202), and one that demonstrates the off-by-1 check for aspect ratios. If this seems wrong to anyone, you can reopen the ticket and tell me so :)

[boonebgorges]

In 34775:

More explicit tests for image_get_intermediate_size().

After [33807], Tests_Image_Intermediate_Size::test_get_intermediate_sizes_by_array_zero_width()
was failing intermittently. This failure was due to the use of a random number
for the image height. When the height was sufficiently large - $height >= 202 -
to change the aspect ratio of the cropped image (based on WP's threshold of a
1px difference), the test passed. And when the height was exactly 200, the
medium image size was hit exactly. The failure occurred only with a height of
201, which is close enough to 200 so that WP determined that the aspect ratio
of the potential crop was close enough to match.

The current changeset splits the test into two, in order to demonstrate the
failure.

See #17626. Fixes #34087.

[SergeyBiryukov]

I was debugging this too the other night, and I concur with comment:2 and [34775].

The previous test did not account for all the cropping logic.

[johnbillion]

In 35345:

Provide a more helpful failure message when Imagick isn't installed. Skipping the tests isn't really an option because Imagick's presence affects several other tests too.

See #34087

[johnbillion]

In 35348:

Remove the explicit Imagick extension test.

See #34087