On Thu, 16 Nov 2023 05:19:43 GMT, Abhishek Kumar <[email protected]> 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
