RFR: 8338103: Stabilize and open source a Swing OGL ButtonResizeTest [v6]

Wed Aug 28 05:27:24 UTC 2024

On Tue, 27 Aug 2024 18:24:26 GMT, Manukumar V S <mvs at openjdk.org> wrote:

>> This PR creates a new test by stabilising and open sourcing a closed test.
>> This test verifies that the OpenGL pipeline does not create artifacts with swing components after window is zoomed to maximum size and then resized back to normal.
>> This test is run twice, with and without the flags "-Dsun.java2d.opengl=True -Dsun.java2d.opengl.fbobject=false" .
>> 
>> This is tested(15 times per platform) in all the available mach5 headful platforms and found to be stable.
>
> Manukumar V S has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Review comment fixed : Added different @test tags for different platforms

test/jdk/javax/swing/JButton/SwingButtonResizeTestWithOpenGL.java line 2:

> 1: /*
> 2:  * Copyright (c) 2015, 2024, Oracle and/or its affiliates. All rights reserved.

I think the copyright year should be just 2024. Looks like the test is added in the repo first time.
Suggestion:

test/jdk/javax/swing/JButton/SwingButtonResizeTestWithOpenGL.java line 29:

> 27: import javax.swing.SwingUtilities;
> 28: import java.awt.Color;
> 29: import java.awt.Dimension;

Please sort the imports. Usually `java.awt` imports are placed before `javax.swing`.

test/jdk/javax/swing/JButton/SwingButtonResizeTestWithOpenGL.java line 53:

> 51:  * resized back to normal.  The test case simulates this operation using
> 52:  * a JButton.  A file image of the component will be saved before and after
> 53:  * window resize. The test passes if both button images are the same.

Suggestion:

 * resized back to normal. The test case simulates this operation using
 * a JButton. A file image of the component will be saved before and after

test/jdk/javax/swing/JButton/SwingButtonResizeTestWithOpenGL.java line 99:

> 97:         button = new JButton("Button A");
> 98:         frame.setLocationRelativeTo(null);
> 99:         frame.setLocation(200, 200);

Isn't it redundant to `setLocation` of frame after calling `setLocationRelativeTo`?

test/jdk/javax/swing/JButton/SwingButtonResizeTestWithOpenGL.java line 133:

> 131:             } else {
> 132:                 System.out.println("Button focus not gained...");
> 133:                 throw new RuntimeException("Can't gain focus on button even after waiting too long..");

Crosses the limit of 80 columns here and few more lines below. Please limit it to 80 columns.

test/jdk/javax/swing/JButton/SwingButtonResizeTestWithOpenGL.java line 145:

> 143:             if (frame.getToolkit().isFrameStateSupported(JFrame.MAXIMIZED_BOTH)) {
> 144:                 robot.waitForIdle();
> 145:                 //maximize frame from normal size

Suggestion:

                // maximize frame from normal size

test/jdk/javax/swing/JButton/SwingButtonResizeTestWithOpenGL.java line 152:

> 150:                 if (frame.getToolkit().isFrameStateSupported(JFrame.NORMAL)) {
> 151:                     System.out.println("Frame is back to normal");
> 152:                     //resize from maximum size to normal

Suggestion:

                    // resize from maximum size to normal

test/jdk/javax/swing/JButton/SwingButtonResizeTestWithOpenGL.java line 159:

> 157:                     bimage2 = getButtonImage();
> 158:                     File file2 = new File("image2.png");
> 159:                     saveButtonImage(bimage2, file2);

We are saving these images even in the case where test passes. Generally, we should save image only in case of test failure for debugging purpose.

test/jdk/javax/swing/JButton/SwingButtonResizeTestWithOpenGL.java line 168:

> 166:                     int numDiffPixels = di.getNumDiffPixels();
> 167:                     if (numDiffPixels != 0) {
> 168:                         throw new RuntimeException("Button renderings are different after window resize, num of Diff Pixels=" + numDiffPixels);

I think the `compare` method can be modified to return `true/false` based on the nDiff value. If  **nDiff is not equal to zero** which you are checking at [L300](https://github.com/openjdk/jdk/blob/5694b6811e06793ddaf1984e909cc0e925d93047/test/jdk/javax/swing/JButton/SwingButtonResizeTestWithOpenGL.java#L300) then you can return `false`, evaluate the return value inside if condition and throw RTE with the num of diff pixels value. If **nDiff is equal to zero** then compare method should return `true` and test should pass.

Code snippet can be changed to something like this...

> System.out.println("Taking the diff of two images, image1 and image2");
>                     // results of image comparison
>                     if (!di.compare(bimage1, bimage2)) {
>                         throw new RuntimeException("Button renderings are different after window resize, num of Diff Pixels=" + di.getNumDiffPixels());
}

test/jdk/javax/swing/JButton/SwingButtonResizeTestWithOpenGL.java line 181:

> 179:             }
> 180:         } finally {
> 181:             disposeFrame();

should be disposed on EDT.

test/jdk/javax/swing/JButton/SwingButtonResizeTestWithOpenGL.java line 185:

> 183:     }
> 184: 
> 185:     //Capture button rendering as a BufferedImage

Suggestion:

    // Capture button rendering as a BufferedImage

test/jdk/javax/swing/JButton/SwingButtonResizeTestWithOpenGL.java line 203:

> 201:     }
> 202: 
> 203:     //Save BufferedImage to PNG file

Suggestion:

    // Save BufferedImage to PNG file

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20532#discussion_r1733987202
PR Review Comment: https://git.openjdk.org/jdk/pull/20532#discussion_r1733967856
PR Review Comment: https://git.openjdk.org/jdk/pull/20532#discussion_r1733970039
PR Review Comment: https://git.openjdk.org/jdk/pull/20532#discussion_r1733971416
PR Review Comment: https://git.openjdk.org/jdk/pull/20532#discussion_r1733990904
PR Review Comment: https://git.openjdk.org/jdk/pull/20532#discussion_r1733988634
PR Review Comment: https://git.openjdk.org/jdk/pull/20532#discussion_r1733988706
PR Review Comment: https://git.openjdk.org/jdk/pull/20532#discussion_r1733997751
PR Review Comment: https://git.openjdk.org/jdk/pull/20532#discussion_r1734007708
PR Review Comment: https://git.openjdk.org/jdk/pull/20532#discussion_r1733992703
PR Review Comment: https://git.openjdk.org/jdk/pull/20532#discussion_r1733991797
PR Review Comment: https://git.openjdk.org/jdk/pull/20532#discussion_r1733992309