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

[Naveen Narayanan]

On Mon, 21 Oct 2024 13:28:37 GMT, Alexey Ivanov <aivanov at openjdk.org> wrote:

>> 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?

Using IMAGE_WIDTH and IMAGE_HEIGHT now.

>> test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 91:
>> 
>>> 89:         robot = new Robot();
>>> 90:         robot.delay(delay);
>>> 91:         robot.waitForIdle();
>> 
>> I guess `robot.waitForIdle()` should be followed by `robot.delay(delay)`. 
>> I agree with @TejeshR13 that you can get rid of `delay` variable, just used once.
>
>> `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.

Taken `delay` variable out now.

>> 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.

Saving the images now on failure.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1809164557
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1809165834
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1809157138