RFR: 8342098: Write a test to compare the images [v6]
Alexey Ivanov
aivanov at openjdk.org
Wed Oct 30 15:18:13 UTC 2024
On Fri, 25 Oct 2024 15:01:49 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 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.
test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 81:
> 79:
> 80: canvas = new ImageCanvas();
> 81: canvas.setBackground(Color.YELLOW);
I recommend setting the preferred size of the canvas instead of setting a size of the frame:
canvas.setPreferredSize(new Dimension(IMAGE_WIDTH + OFFSET * 2,
IMAGE_HEIGHT + OFFSET * 2));
By using `OFFSET * 2` you add a margin on either side of the image on the canvas.
test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 84:
> 82: frame.setLayout(new BorderLayout());
> 83: frame.add(canvas);
> 84: frame.setSize(300, 200);
Suggestion:
frame.pack();
If you set the preferred size for the canvas, just pack the frame.
test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 99:
> 97:
> 98: Rectangle rect = new Rectangle(point.x + 10, point.y + 10, IMAGE_WIDTH,
> 99: IMAGE_HEIGHT);
Suggestion:
Rectangle rect = new Rectangle(point.x + OFFSET, point.y + OFFSET,
IMAGE_WIDTH, IMAGE_HEIGHT);
test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 106:
> 104: String errorMessage = "FAIL : Captured Image is different from "
> 105: + "the real image";
> 106: System.err.println("Test failed");
I agree with @kumarabhi006, `"Test failed"` could be redundant.
test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 118:
> 116: int realPixel;
> 117: int imgWidth = capturedImg.getWidth(null);
> 118: int imgHeight = capturedImg.getHeight(null);
Suggestion:
int imgWidth = capturedImg.getWidth();
int imgHeight = capturedImg.getHeight();
Both images are of type `BufferedImage` which has `getWidth()` and `getHeight()` without parameters.
test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 133:
> 131: System.out.println("Captured pixel ("
> 132: + Integer.toHexString(capturedPixel) + ") at (" + i
> 133: + ", " + j + ") is not equal to real pixel ("
Suggestion:
+ Integer.toHexString(capturedPixel) + ") at "
+ "(" + i + ", " + j + ") is not equal to real pixel ("
Keeping the coordinates close in the code improves readability, doesn't it?
I'd also keep the opening and closing parentheses for pixels on the same line.
test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 145:
> 143: @Override
> 144: public void paint(Graphics g) {
> 145: g.drawImage(realImage, 10, 10, this);
Suggestion:
g.drawImage(realImage, OFFSET, OFFSET, this);
test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 150:
> 148:
> 149: private static void saveImage(BufferedImage image, String fileName) {
> 150: // Save BufferedImage to PNG file
Suggestion:
I think the comment is redundant.
test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 155:
> 153: System.out.println("Saving image : " + image + " to \n"
> 154: + file.getAbsolutePath());
> 155: ImageIO.write(image, "PNG", file);
Suggestion:
ImageIO.write(image, "png", new File(fileName));
The body can be shortened.
(Usually, the format is in lower case; that's what I've seen the most in tests which save images.)
Does printing the format of the image and the absolute path help? I see no value.
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_.
test/jdk/java/awt/Robot/ScreenCaptureRobotTest.java line 165:
> 163: if (frame != null) {
> 164: frame.dispose();
> 165: frame = null;
Assigning `null` is not necessary.
-------------
PR Review: https://git.openjdk.org/jdk/pull/21524#pullrequestreview-2405211906
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1822806853
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1822814897
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1822816416
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1822818768
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1822822354
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1822826053
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1822836347
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1822837524
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1822839304
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1822845059
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1822849256
PR Review Comment: https://git.openjdk.org/jdk/pull/21524#discussion_r1822850523
More information about the client-libs-dev
mailing list