On Thu, 16 Nov 2023 05:19:43 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: > > Review comment fix test/jdk/com/sun/java/swing/plaf/gtk/TestFileChooserSingleDirectorySelection.java line 66: > 64: try { > 65: // create test directory > 66: String tmpDir = System.getProperty("java.io.tmpdir"); This is the purpose of the [*scratch* directory in jtreg](https://openjdk.org/jtreg/faq.html#scratch-directory), and it's automatically [cleaned up after the test](https://openjdk.org/jtreg/writetests.html#cleanFiles): <q cite="https://openjdk.org/jtreg/writetests.html#cleanFiles">jtreg will automatically clean up any files written in the scratch directory.</q> The test is designed for jtreg, so the simplest solution is *to use the current directory*. If someone runs the test directly, without using jtreg, then it's their task to set up the current directory or clean up after the test. test/jdk/com/sun/java/swing/plaf/gtk/TestFileChooserSingleDirectorySelection.java line 88: > 86: testFile.mkdir(); > 87: } > 88: testFile.deleteOnExit(); This is confusing: `testFile` is actually a directory. You could improve the readability of the test by using methods: `createFoldersOnlyDir`, `createFilesOnlyDir`, `populateDirs`, `populateFiles` or something similar — it would make the comments in the code unnecessary. test/jdk/com/sun/java/swing/plaf/gtk/TestFileChooserSingleDirectorySelection.java line 98: > 96: } > 97: } catch (Exception e) { > 98: e.printStackTrace(); The exception should've been propagated: if an exception is thrown, test setup code has failed to prepare the expected environment — the test would likely fail too, then let it fail quickly and report the real reason. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/16674#discussion_r1399678519 PR Review Comment: https://git.openjdk.org/jdk/pull/16674#discussion_r1399677679 PR Review Comment: https://git.openjdk.org/jdk/pull/16674#discussion_r1399680175