RFR: 8319938: com/sun/java/swing/plaf/gtk/TestFileChooserSingleDirectorySelection.java fails with "getSelectedFiles returned empty array for LAF: javax.swing.plaf.metal.MetalLookAndFeel" [v3]

[Alexey Ivanov]

On Wed, 22 Nov 2023 05:42:24 GMT, Abhishek Kumar <abhiscxk at openjdk.org> wrote:

>> The test fails for JFileChooser selection mode set to `DIRECTORIES_ONLY`. For `DIRECTORIES_ONLY `mode, there may not be any directories in home directory and due to that test failed. Added the code to create temporary directories and files for the test.
>> Tested the current change on the machine it failed for multiple times, no failure observed.
>> CI link attached in JBS.
>
> Abhishek Kumar has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Test update

Changes requested by aivanov (Reviewer).

test/jdk/com/sun/java/swing/plaf/gtk/TestFileChooserSingleDirectorySelection.java line 1:

> 1: /*

Since GitHub doesn't allow adding comments to lines which aren't modified:

In `doTesting`, you should get the location and dimensions of the frame on EDT (and for the button too).

The `passed` field is to be declared `volatile`: you modify it on EDT in the button listener but you read its value on main thread.

Basically, you don't need the button, you can run the code in the button action listener directly. Be sure to run it on EDT though.

The listener code can be modified to perform only one volatile write:


            public void actionPerformed(ActionEvent evt) {
                File files[] = fileChooser.getSelectedFiles();
                passed = files.length != 0;
            }

test/jdk/com/sun/java/swing/plaf/gtk/TestFileChooserSingleDirectorySelection.java line 26:

> 24: /*
> 25:  * @test
> 26:  * @bug 6972078 8319938

It's a test bug, no code in JDK is changed as the result — the bugid shouldn't be added.

test/jdk/com/sun/java/swing/plaf/gtk/TestFileChooserSingleDirectorySelection.java line 57:

> 55:     private static File testFile;
> 56:     private static File[] SubDirs;
> 57:     private static File[] subFiles;

You don't use these arrays, they could be local variables; but even there they're not needed — a local variable for a current file or directory is enough.

test/jdk/com/sun/java/swing/plaf/gtk/TestFileChooserSingleDirectorySelection.java line 66:

> 64:         createFilesOnlyDir();
> 65:         populateDirs();
> 66:         populateFiles();

Returning and passing the `File` object would make code flow clearer:
Suggestion:

        testDir = createFoldersOnlyDir();
        populateDirs(testDir);
        testFile = createFilesOnlyDir();
        populateFiles(testFile);


I still find `testFile` confusing because it represents a directory. What about `testDirDirs`, `testDirFiles`?

test/jdk/com/sun/java/swing/plaf/gtk/TestFileChooserSingleDirectorySelection.java line 93:

> 91:             SubDirs[i].mkdir();
> 92:             SubDirs[i].deleteOnExit();
> 93:         }

The array is redundant:
Suggestion:

        for (int i = 0; i < 10; ++i) {
            File subDir = new File(testDir, "subDir_" + (i+1));
            subDir.mkdir();
            subDir.deleteOnExit();
        }

test/jdk/com/sun/java/swing/plaf/gtk/TestFileChooserSingleDirectorySelection.java line 111:

> 109:                     ".txt", new File(testFile.getAbsolutePath()));
> 110:             subFiles[i].deleteOnExit();
> 111:         }

The array is redundant. I also suggest following the same pattern for creating files:
Suggestion:

        for (int i = 0; i < 10; ++i) {
            File subFile = new File(testFile, "subFiles_" + (i+1));
            subFile.createNewFile();
            subFile.deleteOnExit();
        }

test/jdk/com/sun/java/swing/plaf/gtk/TestFileChooserSingleDirectorySelection.java line 143:

> 141:                 createAndShowUI();
> 142:                 fileChooser.setCurrentDirectory(testDir);
> 143:             });

`fileChooser.setFileSelectionMode` that you call below should be called on EDT, I suggest moving it into this block.

This applies to all `check-*` methods.

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

PR Review: https://git.openjdk.org/jdk/pull/16674#pullrequestreview-1763366339
PR Review Comment: https://git.openjdk.org/jdk/pull/16674#discussion_r1414493186
PR Review Comment: https://git.openjdk.org/jdk/pull/16674#discussion_r1414468948
PR Review Comment: https://git.openjdk.org/jdk/pull/16674#discussion_r1414470226
PR Review Comment: https://git.openjdk.org/jdk/pull/16674#discussion_r1414472923
PR Review Comment: https://git.openjdk.org/jdk/pull/16674#discussion_r1414474303
PR Review Comment: https://git.openjdk.org/jdk/pull/16674#discussion_r1414480135
PR Review Comment: https://git.openjdk.org/jdk/pull/16674#discussion_r1414481695