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

[Alexey Ivanov]

On Wed, 30 Oct 2024 14:53:43 GMT, Alexey Ivanov <aivanov 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:         realImage = GraphicsEnvironment.getLocalGraphicsEnvironment()
>> 69:                 .getDefaultScreenDevice().getDefaultConfiguration()
>> 70:                 .createCompatibleImage(IMAGE_WIDTH, IMAGE_HEIGHT);
> 
> Suggestion:
> 
>         realImage = GraphicsEnvironment.getLocalGraphicsEnvironment()
>                 .getDefaultScreenDevice()
>                 .getDefaultConfiguration()
>                 .createCompatibleImage(IMAGE_WIDTH, IMAGE_HEIGHT);
> 
> Don't hide a call and wrap at each chained call — it's easier to track what's happening.

What about this suggestion? Do you disagree?

> test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 122:
> 
>> 120:         if (imgWidth != IMAGE_WIDTH || imgHeight != IMAGE_HEIGHT) {
>> 121:             System.out
>> 122:                     .println("Captured and real images are different in size");
> 
> Suggestion:
> 
>             System.out.println("Captured and real images are different in size");
> 
> Please, don't wrap calls to `println` like this, instead wrap the message if required.
> 
> In this case, wrapping is unnecessary. The line is 81-column long, wrapping it doesn't improve the code readability, quite the opposite.

I'm for changing this and getting rid of wrapping here.

> test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 158:
> 
>> 156:         } catch (IOException ioe) {
>> 157:             throw new RuntimeException(
>> 158:                     "Image save failed : " + ioe.getMessage());
> 
> I would refrain from throwing this exception: it is not important, it is not the reason why the test fails. If you throw this exception here, you'll hide the real reason, that is that _the images are different_.

You still throw `IOException`… without wrapping it into `RuntimeException`.

The suggestion was to ignore `IOException`, maybe printing diagnostic message to `System.err`:


    private static void saveImage(BufferedImage image, String fileName) {
        try {
            ImageIO.write(image, "png", new File(fileName));
        } catch (IOException ignored) {
        }
    }

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1838473018
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1838470504
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1838480434