RFR: 8351884: Refactor bug8033699.java test code

[Alexey Ivanov]

On Wed, 2 Apr 2025 17:34:34 GMT, Rajat Mahajan <rmahajan at openjdk.org> wrote:

> Details:
> Refactored code as requested in the Bug description.
> 
> Tested and verified the test passes.

Overall, it looks good to me.

Except for the minor comments that I left, I can suggest:

- removing `System.out.println` before throwing an error — it adds no value, the error message from the exception is sufficient;
- removing "as expected" from the error message which is implied because it's an error.

test/jdk/javax/swing/JRadioButton/8033699/bug8033699.java line 36:

> 34: import java.awt.event.ActionListener;
> 35: import java.awt.event.KeyEvent;
> 36: import javax.swing.BorderFactory;

I'd rather preserve the blank line between `java.*` and `javax.*` packages. However, the list of imports isn't too long in either block.

test/jdk/javax/swing/JRadioButton/8033699/bug8033699.java line 74:

> 72:                 focusManager = KeyboardFocusManager.getCurrentKeyboardFocusManager();
> 73:             });
> 74: 

Storing the `focusManager` once before calling `testLaF` for the first time is enough.

test/jdk/javax/swing/JRadioButton/8033699/bug8033699.java line 181:

> 179:         mainFrame.getContentPane()
> 180:                  .setLayout(new BoxLayout(mainFrame.getContentPane(),
> 181:                                           BoxLayout.Y_AXIS));

Alternatively, you can create another vertical box, put all the components into it instead of `mainFrame`, and add this vertical box into the frame.

test/jdk/javax/swing/JRadioButton/8033699/bug8033699.java line 301:

> 299:     private static boolean actRB1 = false;
> 300:     private static boolean actRB2 = false;
> 301:     private static boolean actRB3 = false;

These three boolean variables need to be declared volatile, the value is modified on the EDT but their value is read on the main thread.

test/jdk/javax/swing/JRadioButton/8033699/bug8033699.java line 320:

> 318: 
> 319:         hitKey(KeyEvent.VK_DOWN);
> 320:         hitKey(KeyEvent.VK_DOWN);

`addActionListener`, as well as `removeActionListener`, should rather be called on EDT.

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

Changes requested by aivanov (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/24384#pullrequestreview-2776316752
PR Review Comment: https://git.openjdk.org/jdk/pull/24384#discussion_r2049286794
PR Review Comment: https://git.openjdk.org/jdk/pull/24384#discussion_r2049291058
PR Review Comment: https://git.openjdk.org/jdk/pull/24384#discussion_r2049300044
PR Review Comment: https://git.openjdk.org/jdk/pull/24384#discussion_r2049307571
PR Review Comment: https://git.openjdk.org/jdk/pull/24384#discussion_r2049310017