RFR: 8306137: Open source several AWT ScrollPane related tests

[Alexey Ivanov]

On Mon, 24 Apr 2023 17:39:17 GMT, Tejesh R <tr at openjdk.org> wrote:

> Open source few AWT ScrollPane related tests.

Changes requested by aivanov (Reviewer).

test/jdk/java/awt/ScrollPane/RemoveChild/ScrollPaneRemoveAdd.java line 84:

> 82:                     pane.remove(0);
> 83:                     pane.add(button);
> 84:             });

Indentation is incorrect: the body of lambda expression should be indented by four spaces less.

test/jdk/java/awt/ScrollPane/RemoveChild/ScrollPaneRemoveAdd.java line 88:

> 86:             Point buttonLoc = button.getLocationOnScreen();
> 87:             Dimension buttonSize = button.getSize();
> 88:             robot.mouseMove(buttonLoc.x + buttonSize.width / 2, buttonLoc.y + buttonSize.height / 2);

Suggestion:

            robot.mouseMove(buttonLoc.x + buttonSize.width / 2,
                            buttonLoc.y + buttonSize.height / 2);

Without wrapping, the line seems too long. I haven't checked how long it is.

test/jdk/java/awt/ScrollPane/RemoveChild/ScrollPaneRemoveAdd.java line 94:

> 92:             robot.mouseRelease(InputEvent.BUTTON1_DOWN_MASK);
> 93: 
> 94:             Toolkit.getDefaultToolkit().sync();

Shouldn't `sync` be replaced with `Robot.waitForIdle`?

test/jdk/java/awt/ScrollPane/RemoveChild/Semaphore.java line 1:

> 1: public class Semaphore {

If it's left as an external file, it requires its own copyright header. I suggest moving it into `ScrollPaneRemoveAdd.java` as static nested class and then drop `RemoveChild` directory.

Please add a blank line between methods.

test/jdk/java/awt/ScrollPane/ScrollPaneInsets/ScrollPaneExtraScrollBar.java line 27:

> 25:   @test
> 26:   @bug 4152524
> 27:   @summary Test that scroll pane doesn't have scroll bars visible when it is shown for the first time with SCROLLBARS_AS_NEEDED style

Wrap long lines so that they fit into 80-column limit or, at the very least, into 100-column limit.

test/jdk/java/awt/ScrollPane/ScrollPaneInsets/ScrollPaneExtraScrollBar.java line 42:

> 40: 
> 41: public class ScrollPaneExtraScrollBar
> 42: {

Suggestion:

public class ScrollPaneExtraScrollBar {

The opening brace should be on the previous line.

test/jdk/java/awt/ScrollPane/ScrollPaneInsets/ScrollPaneExtraScrollBar.java line 54:

> 52:     public void init() throws Exception {
> 53: 
> 54:        robot = new Robot();

Suggestion:

    }

   public void init() throws Exception {
        robot = new Robot();

A blank line between methods is missing.  
The method starts with an unnecessary blank line.  
The first statement is not indented by four spaces (but only by three).

test/jdk/java/awt/ScrollPane/ScrollPaneInsets/ScrollPaneExtraScrollBar.java line 64:

> 62:                 f.setLocationRelativeTo(null);
> 63:                 f.setVisible(true);
> 64:         });

The lambda expression should be indented by four spaces less.

test/jdk/java/awt/ScrollPane/ScrollPaneInsets/ScrollPaneExtraScrollBar.java line 69:

> 67:     public void start() throws Exception {
> 68:         try {
> 69:             if (System.getProperty("os.name").startsWith("Windows")) {

Should we use [`OSInfo` class](https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/sun/awt/OSInfo.java)?

It's used in [`bug4847375.java`](https://github.com/openjdk/jdk/blob/master/test/jdk/javax/swing/JFileChooser/4847375 /bug4847375.java), [`bug6550546.java`](https://github.com/openjdk/jdk/blob/master/test/jdk/javax/swing/JFileChooser/6550546/bug6550546.java) and others.

The `@requires` tag should be added to the test tags so that it's not selected for other platforms.

To avoid extensive indentation, reverse the condition in the `if` statement and return early.

test/jdk/java/awt/ScrollPane/ScrollPaneLimitation/ScrollPaneLimitation.java line 83:

> 81:                     (os.indexOf("98") == -1) &&
> 82:                     (os.startsWith("Win") || os.startsWith("win") ||
> 83:                             os.startsWith("WIN"))) {

Should use `OSInfo`?

`@requires` tag should be added to the test so that it's not selected for other platforms.

test/jdk/java/awt/ScrollPane/ScrollPaneLimitation/ScrollPaneLimitation.java line 113:

> 111:                     } catch (InterruptedException e) {
> 112:                         throw new RuntimeException("test was interrupted");
> 113:                     }

Suggestion:

                    robot.delay(1000);

Much cleaner, isn't it?

test/jdk/java/awt/ScrollPane/ScrollPaneLimitation/ScrollPaneLimitation.java line 169:

> 167:     }
> 168: 
> 169:     class MyPanel extends Component {

Can be made `static`.
Suggestion:

    private static class MyPanel extends Component {

test/jdk/java/awt/ScrollPane/ScrollPaneWindowsTest/ScrollPaneWindowsTest.java line 91:

> 89:             System.out.println("This test is for Windows 2000/2003/XP only.");
> 90:             return;
> 91:         }

Again, use `OSInfo` or better `@requires` tag.

test/jdk/java/awt/ScrollPane/ScrollPositionIntact/ScrollPositionIntact.java line 77:

> 75:                 } catch (Exception ex) {
> 76:                     throw new RuntimeException("Test failure: interruption?\n\n");
> 77:                 }

It doesn't make sense: you're blocking EDT. It's probably meant to be run on main thread to allow EDT to process events and display the frame.

test/jdk/java/awt/ScrollPane/ScrollPositionIntact/ScrollPositionIntact.java line 97:

> 95:                     }
> 96:                 }
> 97:             });

Why is it `invokeLater`? After you submitted a code to be run on EDT, you exit the test.

In other places in this test, you use lambda expressions instead of anonymous classes, why not here? I think it's better to be consistent and use either lambda or anonymous classes.

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

PR Review: https://git.openjdk.org/jdk/pull/13621#pullrequestreview-1398620929
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1175673089
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1175673980
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1175674660
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1175675939
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1175690233
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1175677434
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1175680065
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1175680662
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1175685964
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1175687475
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1175692384
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1175693400
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1175694722
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1175696461
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1175671146