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

[Alexey Ivanov]

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

Changes requested by aivanov (Reviewer).

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

> 1: /*

Does this test has a value at all?

Does this test verifies the functionality of the `Robot` class? Many other tests rely on `Robot.createScreenCapture` to capture part of the screen. If `Robot` captures something unexpected, all such tests should fail.

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

> 41:  * @summary Verify that the image captured from the screen using a Robot
> 42:  * and the source image are same.
> 43:  * @run main ScreenCaptureRobotTest

Suggestion:

 * @run main/othervm -Dsun.java2d.uiScale=1 ScreenCaptureRobotTest

Otherwise the test would fail if run in a High DPI environment. [Phil mentioned it](https://github.com/openjdk/jdk/pull/21524#discussion_r1805263931).

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

> 67: 
> 68:         Graphics g = realImage.createGraphics();
> 69:         g.setColor(Color.yellow);

Please use upper-case color constants.

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

> 70:         g.fillRect(0, 0, 200, 100);
> 71:         g.setColor(Color.red);
> 72:         g.setFont(new Font("SansSerif", Font.BOLD, 20));

Suggestion:

        g.setFont(new Font(Font.SANS_SERIF, Font.BOLD, 20));

Use the constant for the logical font.

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

> 80:         frame.add(canvas);
> 81:         frame.setSize(300, 200);
> 82:         frame.setLocation(100, 100);

Is it important?
I'd rather put the frame into the centre of the screen as most new tests do.

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

> 82:         frame.setLocation(100, 100);
> 83:         frame.setVisible(true);
> 84:         canvas.requestFocus();

Is `canvas` focusable? I guess it's not, then this statement doesn't make sense.

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

> 92: 
> 93:         Point pnt = canvas.getLocationOnScreen();
> 94:         Rectangle rect = new Rectangle(pnt.x + 10, pnt.y + 10, 200, 100);

Why are coordinates of the start of the canvas are offset by 10 pixels but the size isn't. The image is 200×100, then you capture 10 pixels on the right and bottom which belong to whatever is on the screen in this area.

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

> 135:         @Override
> 136:         public void paint(Graphics g) {
> 137:             g.drawImage(displayImage, 10, 10, this);

You can use `realImage` here. Both fields point to the same object anyway.

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

PR Review: https://git.openjdk.org/jdk/pull/21524#pullrequestreview-2382115196
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808870689
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808874757
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808826430
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808829862
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808832519
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808833268
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808839354
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808862076