RFR: 8306137: Open source several AWT ScrollPane related tests

[Tejesh R]

On Mon, 24 Apr 2023 18:57:45 GMT, Alexey Ivanov <aivanov at openjdk.org> wrote:

>> Open source few AWT ScrollPane related tests.
>
> 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.

Updated.

> 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.

Updated.

> 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.

Updated.

> 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.

Updated.

> 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).

Updated.

> 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.

Updated.

> 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.

Updated to @requires.

> 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.

Updated.

> 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?

Updated.

> 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 {

Updated.

> 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.

Yeah, I've added robot delay in-between EDT.

> 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.

Updated to `invokeAndWait`() with lambda expressions.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1176031414
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1176031323
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1176031181
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1176031107
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1176031069
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1176031009
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1176029967
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1176029791
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1176029424
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1176029342
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1176028312
PR Review Comment: https://git.openjdk.org/jdk/pull/13621#discussion_r1176031802