On Wed, 22 Nov 2023 05:42:24 GMT, Abhishek Kumar <abhis...@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