On Mon, 21 Oct 2024 13:28:37 GMT, Alexey Ivanov <[email protected]> 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
