Off by one error in Imagick testsuite

[Fuegas]

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)

[Fuegas]

Proposed fix for Imagick testsuite

[kirasong]

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.

[Fuegas]

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

[kirasong]

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/

[kirasong]

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.

[kirasong]

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?

[Fuegas]

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

[kirasong]

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

[kirasong]

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.

[desrosj]

Sorry to reopen this old ticket. In working on #48301, I discovered that this blocks the ability to backport the local Docker environment. It looks like it needs to be brought all the way to the 3.7 branch.

[desrosj]

In 49517:

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.
Merges [45067] to the 5.1 branch.
See #46073, #48301.

[desrosj]

In 49518:

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.
Merges [45067] to the 5.0 branch.
See #46073, #48301.

[desrosj]

In 49519:

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.
Merges [45067] to the 4.9 branch.
See #46073, #48301.

[desrosj]

In 49520:

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.
Merges [45067] to the 4.8 branch.
See #46073, #48301.

[desrosj]

In 49521:

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.
Merges [45067] to the 4.7 branch.
See #46073, #48301.

[desrosj]

In 49522:

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.
Merges [45067] to the 4.6 branch.
See #46073, #48301.

[desrosj]

Going to leave this open for now until the additional local Docker environment blockers for the branches < 4.6 can be resolved.

[desrosj]

Re-closing for now. Can backport further when the blockers are lifted.