WordPress.org

Make WordPress Core

Opened 11 months ago

Closed 9 months ago

#46073 closed defect (bug) (fixed)

Off by one error in Imagick testsuite

Reported by: Fuegas Owned by: mikeschroder
Milestone: 5.2 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-patch
Focuses: Cc:
PR Number:

Description

Since a while we're seeing the test suite fail on our platform ( https://make.wordpress.org/hosting/test-results/r44689/savviibot-r44689/ ). We investigated the cause of this failure and saw the following output:

There were 2 failures:

1) Tests_Image_Editor_Imagick::test_rotate
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    'r' => 127
-    'g' => 179
-    'b' => 229
+    'r' => 126
+    'g' => 178
+    'b' => 228
     'a' => 1
 )

.../wp-test-runner/tests/phpunit/tests/image/editorImagick.php:436

2) Tests_Image_Editor_Imagick::test_flip
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    'r' => 127
-    'g' => 179
-    'b' => 229
+    'r' => 126
+    'g' => 178
+    'b' => 228
     'a' => 1
 )

.../wp-test-runner/tests/phpunit/tests/image/editorImagick.php:455

We took a closer look at the tests and found the following code in tests/phpunit/tests/image/editorImagick.php (starting at line 420):

<?php
  /**
   * Test rotating an image 180 deg
   */
  public function test_rotate() {
    $file = DIR_TESTDATA . '/images/gradient-square.jpg';

    $imagick_image_editor = new WP_Image_Editor_Imagick( $file );
    $imagick_image_editor->load();

    $property = new ReflectionProperty( $imagick_image_editor, 'image' );
    $property->setAccessible( true );

    $color_top_left = $property->getValue( $imagick_image_editor )->getImagePixelColor( 1, 1 )->getColor();

    $imagick_image_editor->rotate( 180 );

    $this->assertEquals( $color_top_left, $property->getValue( $imagick_image_editor )->getImagePixelColor( 99, 99 )->getColor() );
  }

  /**
   * Test flipping an image
   */
  public function test_flip() {
    $file = DIR_TESTDATA . '/images/gradient-square.jpg';

    $imagick_image_editor = new WP_Image_Editor_Imagick( $file );
    $imagick_image_editor->load();

    $property = new ReflectionProperty( $imagick_image_editor, 'image' );
    $property->setAccessible( true );

    $color_top_left = $property->getValue( $imagick_image_editor )->getImagePixelColor( 1, 1 )->getColor();

    $imagick_image_editor->flip( true, false );

    $this->assertEquals( $color_top_left, $property->getValue( $imagick_image_editor )->getImagePixelColor( 0, 99 )->getColor() );
  }

The tests uses an image that is 100 x 100 pixels. So the first pixel is at (0, 0) and on the far end that is (99, 99). The rotation test compares the color of (1, 1) with the color at (99, 99) which is actually one pixel too far on x-axis and one too far on the y-axis. The second test flips the image on the horizontal axis and tests the color of (1, 1) with (0, 99). The movement of one pixel on the x-axis has no effect as the image is a linear gradient, but for consistence I'd suggest using X = 1 for the sampling and comparison. However, the comparison on row 99 is off by one. I've added a diff with a proposed fix, if I run the test suite with this patch the suite completes successfully:

$ php phpunit.phar -c phpunit.xml.dist tests/phpunit/tests/image/editorImagick.php

PHP version: 7.2.14

Installing...
Running as single site... To run multisite, use -c tests/phpunit/multisite.xml
Not running ajax tests. To execute these, use --group ajax.
Not running ms-files tests. To execute these, use --group ms-files.
Not running external-http tests. To execute these, use --group external-http.
PHPUnit 5.7.27 by Sebastian Bergmann and contributors.

..............                                                    14 / 14 (100%)

You should really fix these slow tests (>150ms)...
 1. 249ms to run Tests_Image_Editor_Imagick:test_multi_resize


Time: 8.95 seconds, Memory: 32.25MB

OK (14 tests, 43 assertions)

Attachments (3)

imagick-test.diff (929 bytes) - added by Fuegas 11 months ago.
Proposed fix for Imagick testsuite
46073.diff (1.9 KB) - added by mikeschroder 9 months ago.
one-blue-pixel-100x100.png (320 bytes) - added by mikeschroder 9 months ago.

Download all attachments as: .zip

Change History (13)

@Fuegas
11 months ago

Proposed fix for Imagick testsuite

#1 @pento
11 months ago

  • Version trunk deleted

#2 @johnbillion
11 months ago

  • Component changed from Build/Test Tools to Media
  • Keywords has-patch added

#3 @mikeschroder
11 months ago

Hi! Thanks so much for the report!

Just as a quick note, this is passing for me locally and looks to be passing on a couple other hosts with Imagick (noticing Human Made and DreamHost, on a quick glance):
https://make.wordpress.org/hosting/test-results/

This makes me wonder if it's specific to environment somehow.
I'll see if I can reproduce with the versions of the packages reported on that page.

#4 @Fuegas
11 months ago

Hi @mikeschroder, thanks for looking into it. If I can help in some way please let me know.

#5 @mikeschroder
11 months ago

Still looking into this, but wanted to add this information about last working/first failing revision, since the PHP version and extension information might be helpful for debugging:

Last working: https://make.wordpress.org/hosting/test-results/r43678/savviibot-r43678/

First failing: https://make.wordpress.org/hosting/test-results/r44664/savviibot-r44664/

#6 @mikeschroder
11 months ago

  • Milestone changed from Awaiting Review to Future Release

I agree your logic is sound here, and we should change it. I prefer the option of using the exact corners.

Right now, it's passing for me locally both before and after the patch, which is a bit confusing.

The next step for me is that I think it'd be helpful to have an image where it's more clear what's happening -- thinking a square with a single pixel in the corner that is another color, for instance. I'm not sure if that'll be exact enough in JPG, but it'd help both with troubleshooting the test and having it fail properly in the future if it is.

If you get a chance to make the image first, feel free. If not, this is on my list, since I'd love to figure this out and see it passing properly.

@mikeschroder
9 months ago

#7 @mikeschroder
9 months ago

  • Keywords reporter-feedback added
  • Owner set to mikeschroder
  • Status changed from new to assigned

I was finally able to reproduce this, and think this should fix it.

I uploaded 46073.diff with 0->99 as the values, and one-blue-pixel-100x100.png which has exactly one blue pixel in the corner. I went with PNG because it seemed to be more likely to remain a single pixel in the correct color than JPEG.

Does this fix the issue for you?

#8 @Fuegas
9 months ago

I can confirm this fixed our issue:

$ php phpunit.phar -v tests/phpunit/tests/image/editorImagick.php

PHP version: 7.2.15

Installing...
Running as single site... To run multisite, use -c tests/phpunit/multisite.xml
Not running ajax tests. To execute these, use --group ajax.
Not running ms-files tests. To execute these, use --group ms-files.
Not running external-http tests. To execute these, use --group external-http.
PHPUnit 5.7.27 by Sebastian Bergmann and contributors.

..............                                                    14 / 14 (100%)

You should really fix these slow tests (>150ms)...
 1. 314ms to run Tests_Image_Editor_Imagick:test_multi_resize


Time: 11.77 seconds, Memory: 32.25MB

#9 @mikeschroder
9 months ago

  • Keywords reporter-feedback removed
  • Milestone changed from Future Release to 5.2

Thanks so much @Fuegas! I'll get this committed.

#10 @mikeschroder
9 months ago

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

In 45067:

Media: Improve and fix rotate/flip image tests

Fix off-by-one error in pixel color checks for rotate and flip image tests.
Change to using PNG with single pixel to ensure that errors are caught in the future, rather than lost in JPEG noise.

Props Fuegas, mikeschroder.
Fixes #46073.

Note: See TracTickets for help on using tickets.