On Mon, 21 Oct 2024 08:26:54 GMT, Manukumar V S <m...@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 125:
>
>> 123: capturedPixel = capturedImg.getRGB(i, j);
>> 124: realPixel = realImg.getRGB(i, j);
>> 125: if (capturedPixel != realPixel) {
>
> Suggestion:
>
> if (capturedPixel != realPixel) {
> System.out.println("Captured pixel ("+capturedPixel+") at
> (" + i + ", " + j + ") is not equal to real pixel (" + realPixel + ")");
Yes, printing the values would absolutely help.
Please, print pixels in hex, it'll make more sense:
[`Integer.toHexString`](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/Integer.html#toHexString(int)).
> test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 133:
>
>> 131: return result;
>> 132: }
>> 133:
>
> Suggestion:
>
> //Save BufferedImage to PNG file
> private static void saveImage(BufferedImage image, String fileName) {
> if (image != null) {
> try {
> File file = new File(fileName);
> System.out.println("Saving button image to " +
> file.getAbsolutePath());
> ImageIO.write(image, "PNG", file);
> } catch (Exception e) {
> throw new RuntimeException("Could not write image file");
> }
> } else {
> throw new RuntimeException("BufferedImage was set to null");
> }
> }
Yes, please save the images, especially when comparison fails.
I think handling `image != null` is not needed here. If `null` is passed, the
test will fail with `NullPointerException`, and it's acceptable. Without the
`if` statement, the code to save the image would be cleaner.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808850177
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1808864929
