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

[Abhishek Kumar]

On Fri, 18 Oct 2024 15:54:24 GMT, Naveen Narayanan <duke at openjdk.org> wrote:

>> This testcase checks for the following:
>> 
>> 1. An image is drawn on the screen and a portion of it is captured using a Robot. It is tested by comparing whether the captured image is same as the source image. 
>> 
>> Testing:
>> Tested using Mach5(20 times per platform) in MacOS, Linux and Windows. Seen all tests passing.
>
> 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.

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.

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.

test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 93:

> 91:         robot.waitForIdle();
> 92: 
> 93:         Point pnt = canvas.getLocationOnScreen();

should be accessed on EDT.

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.

test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 107:

> 105:             System.out.println("\nCaptured Image is same as actual Image");
> 106:             System.out.println("Test passed");
> 107:         }

May not be required if test passes.

test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 111:

> 109: 
> 110:     private static boolean compareImages(BufferedImage capturedImg,
> 111:             BufferedImage realImg) {

`realImg` is a class variable, not required to pass as an argument.

test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 119:

> 117: 
> 118:         imgWidth = capturedImg.getWidth(null);
> 119:         imgHeight = capturedImg.getHeight(null);

These variables can be removed and `capturedImg.getWidth(null)` should replace `imgWidth` in for-loop condition. same for `imgHeight` also.

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.

test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 127:

> 125:                 if (capturedPixel != realPixel) {
> 126:                     result = false;
> 127:                     return result;

can be replace by `return false`, no need of `result` variable then.

test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 131:

> 129:             }
> 130:         }
> 131:         return result;

Suggestion:

        return true;

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808081942
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808080720
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808084152
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808084265
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808085750
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808086184
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808086674
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808088631
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808089744
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808089131
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808089179