RFR: 8342098: Write a test to compare the images [v3]

[Alexey Ivanov]

On Mon, 21 Oct 2024 04:16:54 GMT, Abhishek Kumar <abhiscxk at openjdk.org> wrote:

>> Naveen Narayanan has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   8342098: Updated review comments
>
> test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 70:
> 
>> 68:         Graphics g = realImage.createGraphics();
>> 69:         g.setColor(Color.yellow);
>> 70:         g.fillRect(0, 0, 200, 100);
> 
> You may define `rect_width` and `rect_height` variable with 200 and 100 respectively.
> These numbers are used more than once and you are trying to avoid magic numbers for better practice.

These are `IMAGE_WIDTH` and `IMAGE_HEIGHT` correspondingly, right?

> test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 78:
> 
>> 76: 
>> 77:         canvas = new ImageCanvas();
>> 78:         canvas.setBackground(Color.YELLOW);
> 
> Use either `Color.yellow` or `Color.YELLOW` for consistency.

The latter is better.

> `robot.waitForIdle()` should be followed by `robot.delay(delay)`.

Yes.

> you can get rid of delay variable, just used once.

I don't mind a constant to control the delay.

> test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 102:
> 
>> 100:             String errorMessage = "FAIL : Captured Image is different from "
>> 101:                     + "the actual image";
>> 102:             System.err.println("Test failed");
> 
> I think you should save the images if image comparison fails, they can be used for diagnostic purpose.
> No need to define a `errorMessage` variable, can throw the error message directly.
> 
> `System.err.println("Test failed");` look redundant to me.

Absolutely right! Save the images — it'll make your life easier when you need to analyse the test failure.

> test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 125:
> 
>> 123:                 capturedPixel = capturedImg.getRGB(i, j);
>> 124:                 realPixel = realImg.getRGB(i, j);
>> 125:                 if (capturedPixel != realPixel) {
> 
> To optimize further, `capturedImg.getRGB(i, j)` and `realImg.getRGB(i, j)` can be directly evaluated inside if condition.

Yet, you'll be unable to print values unless you use `getRGB` again.

> test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 131:
> 
>> 129:             }
>> 130:         }
>> 131:         return result;
> 
> Suggestion:
> 
>         return true;

In fact, image comparison can be borrowed from the `Util` class (in Swing part):

https://github.com/openjdk/jdk/blob/1f3574855e79221739d8800235583b7c47ebae97/test/jdk/javax/swing/regtesthelpers/Util.java#L59-L63

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808827599
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808830534
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808835292
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808841275
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808846337
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808857931