On Thu, 14 Sep 2023 16:35:44 GMT, Damon Nguyen <dngu...@openjdk.org> wrote:
> These are the tests being converted:
>
> javax/swing/JTabbedPane/4703690/bug4703690.java
> javax/swing/JTabbedPane/GetComponentAt/GetComponentAtTest.java
> javax/swing/JTabbedPane/ReplaceCompTab/ReplaceCompTab.java
> javax/swing/JTextArea/4849868/bug4849868.java
> javax/swing/JTextField/4244613/bug4244613.java
Changes requested by aivanov (Reviewer).
test/jdk/javax/swing/JTabbedPane/GetComponentAtTest.java line 48:
> 46:
> 47: public static void main(String[] args) throws InterruptedException,
> 48: InvocationTargetException, AWTException {
Suggestion:
public static void main(String[] args) throws Exception {
should be enough.
test/jdk/javax/swing/JTabbedPane/ReplaceCompTab.java line 78:
> 76: throw new RuntimeException("Adding first null " +
> 77: "component failed:");
> 78: }
Suggestion:
} catch (Exception e) {
throw new RuntimeException("Adding first null " +
"component failed:", e);
}
Pass the original exception as the `cause` parameter if you catch an exception
to provide a more specific error message.
test/jdk/javax/swing/JTabbedPane/bug4703690.java line 47:
> 45:
> 46: public class bug4703690 {
> 47: static JFrame fr;
Suggestion:
static JFrame frame;
It's global after all, it doesn't save typing, yet improves the readability.
test/jdk/javax/swing/JTabbedPane/bug4703690.java line 49:
> 47: static JFrame fr;
> 48: static JTabbedPane tabbedPane;
> 49: static JPanel panel;
The `panel` field can be local variable.
test/jdk/javax/swing/JTabbedPane/bug4703690.java line 54:
> 52: static volatile boolean focusButtonTwo = false;
> 53: static volatile boolean switchToTabTwo = false;
> 54: static volatile boolean focusButtonOne = false;
Assigning the default value is redundant.
Use CountDownLatch instead of boolean values:
public void execute() {
// Create robot
two.requestFocus();
if (!focusButtonTwo.await(1, TimeUnit.SECONDS)) {
throw new RuntimeException("Button two didn't receive focus");
}
// Switch to tab two
// do the click
if (!switchToTabTwo.await(1, TimeUnit.SECONDS)) {
throw new RuntimeException("Switching to tab two failed");
}
// Switch to tab one
// do the click
if (!focusButtonOne.await(1, TimeUnit.SECONDS)) {
throw new RuntimeException("The 'Button 1' button doesn't have
focus");
}
}
The test completes much faster because there are no unneeded delays.
test/jdk/javax/swing/JTabbedPane/bug4703690.java line 86:
> 84: tabbedPane.addChangeListener(e -> {
> 85: if (tabbedPane.getSelectedIndex() == 1) {
> 86: switchToTabTwo = true;
The value of `switchToTabTwo` is never read.
test/jdk/javax/swing/JTabbedPane/bug4703690.java line 92:
> 90: fr.setBounds(10, 10, 200, 200);
> 91: fr.setVisible(true);
> 92: fr.setLocationRelativeTo(null);
Suggestion:
fr.setSize(200, 200);
fr.setLocationRelativeTo(null);
fr.setVisible(true);
test/jdk/javax/swing/JTabbedPane/bug4703690.java line 110:
> 108: robot.setAutoDelay(50);
> 109: robot.delay(1000);
> 110: two.requestFocus();
It may need to be called on EDT too.
Does it make sense to wait till the button receives focus?
I assume the following actions depend on this to happen.
test/jdk/javax/swing/JTabbedPane/bug4703690.java line 123:
> 121: });
> 122:
> 123: robot.delay(1000);
This delay seems to be redundant.
test/jdk/javax/swing/JTabbedPane/bug4703690.java line 136:
> 134: });
> 135:
> 136: robot.delay(1000);
This delay seems redundant.
test/jdk/javax/swing/JTabbedPane/bug4703690.java line 143:
> 141: } catch (Exception t) {
> 142: throw new RuntimeException("Test failed", t);
> 143: }
This catch is redundant — declare the method to throw `Exception` and let them
propagate to `main` which, in its turn, `throws Exception`.
test/jdk/javax/swing/JTextArea/bug4849868.java line 54:
> 52:
> 53: public static void main(String[] args) throws InterruptedException,
> 54: InvocationTargetException, AWTException {
Suggestion:
public static void main(String[] args) throws Exception {
A generic `Exception` is enough for tests.
test/jdk/javax/swing/JTextArea/bug4849868.java line 70:
> 68: f.setSize(300, 300);
> 69: f.setVisible(true);
> 70: f.setLocationRelativeTo(null);
Suggestion:
f.setSize(300, 300);
f.setLocationRelativeTo(null);
f.setVisible(true);
test/jdk/javax/swing/JTextArea/bug4849868.java line 73:
> 71: });
> 72:
> 73: robot.delay(1000);
Suggestion:
robot.waitForIdle();
robot.delay(1000);
test/jdk/javax/swing/JTextArea/bug4849868.java line 76:
> 74:
> 75: SwingUtilities.invokeAndWait(() -> p =
> 76: textArea.getLocationOnScreen());
Suggestion:
SwingUtilities.invokeAndWait(() ->
p = textArea.getLocationOnScreen());
I am absolutely sure it's better not to break the assignment in lambda
expressions, especially when the variable name is a single letter.
-------------
PR Review: https://git.openjdk.org/jdk/pull/15747#pullrequestreview-1630874424
PR Review Comment: https://git.openjdk.org/jdk/pull/15747#discussion_r1328615518
PR Review Comment: https://git.openjdk.org/jdk/pull/15747#discussion_r1328634584
PR Review Comment: https://git.openjdk.org/jdk/pull/15747#discussion_r1328672086
PR Review Comment: https://git.openjdk.org/jdk/pull/15747#discussion_r1328672403
PR Review Comment: https://git.openjdk.org/jdk/pull/15747#discussion_r1328635652
PR Review Comment: https://git.openjdk.org/jdk/pull/15747#discussion_r1328649916
PR Review Comment: https://git.openjdk.org/jdk/pull/15747#discussion_r1328640500
PR Review Comment: https://git.openjdk.org/jdk/pull/15747#discussion_r1328643194
PR Review Comment: https://git.openjdk.org/jdk/pull/15747#discussion_r1328644373
PR Review Comment: https://git.openjdk.org/jdk/pull/15747#discussion_r1328646166
PR Review Comment: https://git.openjdk.org/jdk/pull/15747#discussion_r1328675150
PR Review Comment: https://git.openjdk.org/jdk/pull/15747#discussion_r1328685659
PR Review Comment: https://git.openjdk.org/jdk/pull/15747#discussion_r1328684987
PR Review Comment: https://git.openjdk.org/jdk/pull/15747#discussion_r1328736989
PR Review Comment: https://git.openjdk.org/jdk/pull/15747#discussion_r1328734751
