RFR: 8160755: bug6492108.java test fails with exception Image comparison failed at (0, 0) for image 4 in GTK L&F [v5]

[Abhishek Kumar]

On Tue, 4 Jun 2024 13:45:44 GMT, Alexey Ivanov <aivanov at openjdk.org> wrote:

>> Abhishek Kumar has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Code formatting update
>
> src/java.desktop/share/classes/com/sun/java/swing/plaf/gtk/GTKStyle.java line 1:
> 
>> 1: /*
> 
> You should update the copyright year.

Updated.

> test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 64:
> 
>> 62:         } catch (Exception e) {
>> 63:             throw new SkippedException("GTK LAF is not supported on this system;" +
>> 64:                     " test passes");
> 
> Suggestion:
> 
>             throw new SkippedException("GTK LAF is not supported on this system");
> 
> I'd remove the additional message. I believe it was `println` before.

Updated.

> test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 116:
> 
>> 114:     }
>> 115: 
>> 116:     private void onBackgroundThread20() {
> 
> This method should run on EDT because it accesses the state of the components.

Updated to run this on EDT.

> test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 133:
> 
>> 131:                 Rectangle testRect =
>> 132:                     new Rectangle(loc.x, loc.y,
>> 133:                                   test.getWidth(), test.getHeight());
> 
> Suggestion:
> 
>                 loc = test.getLocationOnScreen();
>                 Rectangle testRect = new Rectangle(test.getLocationOnScreen(),
>                                                    test.getSize());
> 
> Choose one style and adjust both statements, `refRect` and `testRect`.

Updated to use same style for both places.

> test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 136:
> 
>> 134:                 BufferedImage testimg = robot.createScreenCapture(testRect);
>> 135: 
>> 136:                 if (refimg.getWidth() != testimg.getWidth()
> 
> I suggest moving the part that compares the images into a helper method. Alternatively, you can use [`Util.compareBufferedImages`](https://github.com/openjdk/jdk/blob/8d3de45f4dfd60dc4e2f210cb0c085fcf6efb8e2/test/jdk/javax/swing/regtesthelpers/Util.java#L59-L80).

Ok.. Updated [Util.compareBufferedImages](https://github.com/openjdk/jdk/blob/8d3de45f4dfd60dc4e2f210cb0c085fcf6efb8e2/test/jdk/javax/swing/regtesthelpers/Util.java#L59-L80) to compare images.

> test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 137:
> 
>> 135: 
>> 136:                 if (refimg.getWidth() != testimg.getWidth()
>> 137:                    || refimg.getHeight() != testimg.getHeight())
> 
> Suggestion:
> 
>                 if (refimg.getWidth() != testimg.getWidth()
>                     || refimg.getHeight() != testimg.getHeight())
> 
> One more space, it should be aligned to the inside of the parenthesis.

Used Util to compare images, so these lines are removed now.

> test/jdk/com/sun/java/swing/plaf/gtk/bug6492108.java line 148:
> 
>> 146:                         if (refPixel != testPixel) {
>> 147:                             fail("Image comparison failed at (" +
>> 148:                                  x + "," + y + ") for image " + index);
> 
> Adding `count` and `k` to diagnostic message could help identifying the failing case quicker.

Don't find adding `k` to diagnostic message useful.. failure message is more relevant now about the images which are compared.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1626152744
PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1626146072
PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1626148142
PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1626150340
PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1626150884
PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1626149792
PR Review Comment: https://git.openjdk.org/jdk/pull/19381#discussion_r1626152392