WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 11 months ago

#46073 closed defect (bug) (fixed)

Off by one error in Imagick testsuite

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

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 3 years ago.
Proposed fix for Imagick testsuite
46073.diff (1.9 KB) - added by mikeschroder 3 years ago.
one-blue-pixel-100x100.png (320 bytes) - added by mikeschroder 3 years ago.

Download all attachments as: .zip

Change History (24)

@Fuegas
3 years ago

Proposed fix for Imagick testsuite

#1 @pento
3 years ago

  • Version trunk deleted

#2 @johnbillion
3 years ago

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

#3 @mikeschroder
3 years 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
3 years ago

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

#5 @mikeschroder
3 years 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
3 years 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
3 years ago

#7 @mikeschroder
3 years 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
3 years 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
3 years ago

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

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

#10 @mikeschroder
3 years 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.

#11 @desrosj
12 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

#12 @desrosj
12 months ago

  • Owner changed from mikeschroder to desrosj
  • Status changed from reopened to assigned

#13 @desrosj
12 months ago

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.

#14 @desrosj
12 months ago

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.

#15 @desrosj
12 months ago

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.

#16 @desrosj
12 months ago

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.

#17 @desrosj
12 months ago

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.

#18 @desrosj
12 months ago

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.

#19 @desrosj
12 months ago

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

This ticket was mentioned in Slack in #core by pbiron. View the logs.


11 months ago

#21 @desrosj
11 months ago

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

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

Note: See TracTickets for help on using tickets.