[wp-trac] [WordPress Trac] #34087: Tests_Image_Intermediate_Size::test_get_intermediate_sizes_by_array_zero_width fails when the random value is 201

[WordPress Trac]

#34087: Tests_Image_Intermediate_Size::test_get_intermediate_sizes_by_array_zero_width
fails when the random value is 201
--------------------------+------------------
 Reporter:  jorbin        |       Owner:
     Type:  defect (bug)  |      Status:  new
 Priority:  normal        |   Milestone:  4.4
Component:  Media         |     Version:
 Severity:  normal        |  Resolution:
 Keywords:  2nd-opinion   |     Focuses:
--------------------------+------------------
Changes (by boonebgorges):

 * keywords:   => 2nd-opinion


Comment:

 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.

--
Ticket URL: <https://core.trac.wordpress.org/ticket/34087#comment:2>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform