RFR: 8281988: Create a regression test for JDK-4618767 [v2]

[Alexey Ivanov]

On Fri, 18 Feb 2022 09:15:43 GMT, Manukumar V S <mvs at openjdk.org> wrote:

>> Create a regression test for [JDK-4618767](https://bugs.openjdk.java.net/browse/JDK-4618767)
>> 
>> Issue identified in [JDK-4618767](https://bugs.openjdk.java.net/browse/JDK-4618767):
>> Typing a letter while a JList has focus now makes the selection jump to the first/next node/item whose text starts with that letter even though that letter is accompanied by modifier keys such as ALT or CTRL.
>> 
>> Fix:
>> Only enable JList letter navigation when the user
>> doesn't press any modifier keys such as ALT or CTRL.
>> 
>> Testing:
>> I have verified this test with JDK 1.4.0 and JDK 1.4.1 .
>> The issue is reproducible using the test with JDK 1.4.0 , where the bug was originally reported and the test passed in JDK 1.4.1 where the issue was fixed.
>> I have tested it in Mac and Windows platforms multiple times and it passed everywhere.
>
> Manukumar V S has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Wait until focus is on list, changed the mnemonics for opening menu, added menulistener, caught UnsupportedLookAndFeelException and ignore LnF

Changes requested by aivanov (Reviewer).

test/jdk/javax/swing/JList/4618767/JListSelectedElementTest.java line 53:

> 51: public class JListSelectedElementTest {
> 52: 
> 53:     private static final int MENU = KeyEvent.VK_F;

Suggestion:

    private static final int FILE_MENU = KeyEvent.VK_F;

or `FILE_MENU_MNEMONIC`, or use `KeyEvent.VK_F` directly as you do for other keys.

Otherwise, it looks as if you pressing a Menu key, `KeyEvent.VK_CONTEXT_MENU`.

test/jdk/javax/swing/JList/4618767/JListSelectedElementTest.java line 68:

> 66:         robot.setAutoDelay(200);
> 67: 
> 68:         final boolean isAMac = System.getProperty("os.name").toLowerCase().contains("os x");

Suggestion:

        final boolean isMac = System.getProperty("os.name")
                                    .toLowerCase()
                                    .contains("os x");

test/jdk/javax/swing/JList/4618767/JListSelectedElementTest.java line 96:

> 94:                         throw new RuntimeException("Waited for long, but can't get focus for list");
> 95:                     }
> 96:                 }

Maybe you should wait for `FocusEvent.FOCUS_GAINED` event?

In some way, requesting focus to list is redundant: it's the only component in the frame, there's no other component to get the focus.

test/jdk/javax/swing/JList/4618767/JListSelectedElementTest.java line 103:

> 101: 
> 102:                 // Assertion check to verify that selected node is 'bill'
> 103:                 String elementSel = list.getSelectedValue();

`list.getSelectedValue()` must be called on EDT.

test/jdk/javax/swing/JList/4618767/JListSelectedElementTest.java line 120:

> 118:                     robot.keyRelease(KeyEvent.VK_ALT);
> 119:                     robot.keyRelease(KeyEvent.VK_CONTROL);
> 120:                     robot.keyRelease(MENU);

I suggest adding a helper method or using an existing one from reghelpers/Utils class:


    public static void hitKeys(Robot robot, int... keys) {
        for (int i = 0; i < keys.length; i++) {
            robot.keyPress(keys[i]);
        }

        for (int i = keys.length - 1; i >= 0; i--) {
            robot.keyRelease(keys[i]);
        }
    }


If you copy the implementation, you can use your robot field instead of passing it as the parameter.

Also, it's a good practice to release in the reverse order to pressing them, which this method will help to achieve.

test/jdk/javax/swing/JList/4618767/JListSelectedElementTest.java line 121:

> 119:                     robot.keyRelease(KeyEvent.VK_CONTROL);
> 120:                     robot.keyRelease(MENU);
> 121:                 }else{

Suggestion:

                } else {

Spaces missing.

test/jdk/javax/swing/JList/4618767/JListSelectedElementTest.java line 129:

> 127: 
> 128:                 waitCount = 0;
> 129:                 while (!menuSelected) {

A `CountDownLatch` instead of polling loop?

test/jdk/javax/swing/JList/4618767/JListSelectedElementTest.java line 165:

> 163:     private static void createUI() {
> 164:         frame = new JFrame();
> 165:         list = new JList(new String[]{"anaheim", "bill", "chicago", "dingo", "ernie", "freak"});

Suggestion:

        list = new JList<>(new String[]{"anaheim", "bill", "chicago", "dingo", "ernie", "freak"});

test/jdk/javax/swing/JList/4618767/JListSelectedElementTest.java line 174:

> 172:                                  public void menuSelected(MenuEvent e) {
> 173:                                      menuSelected = true;
> 174:                                  }

For anonymous classes, you use the regular indent of 4 spaces:


        menu.addMenuListener(new MenuListener() {
            @Override
            public void menuSelected(MenuEvent e) {
                menuSelected = true;
            }

             // Other methods
        });

test/jdk/javax/swing/JList/4618767/JListSelectedElementTest.java line 189:

> 187:         frame.setJMenuBar(menuBar);
> 188:         frame.setContentPane(list);
> 189:         frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);

Suggestion:

        frame.setDefaultCloseOperation(JFrame.DISPOSE_ON_CLOSE);

You should never call `System.exit` from jtreg tests.

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

PR: https://git.openjdk.java.net/jdk/pull/7496