RFR: 7903953: Use scratch directory for JUnit's TempDir annotation

[Christian Stein]

On Mon, 24 Feb 2025 09:38:15 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Please review this change to use `jtreg`'s scratch directory for JUnit's `@TempDir` annotation.
>> 
>> Prior to this change, the standard implementation of JUnit Jupiter using the system default temporary directory is active, effectively decoupling the location of "interesting on failure" files from `jtreg`:
>>> When `jtreg` executes a test, the current directory for the test is set to a scratch directory so that the test can easily write any temporary files.
>> 
>> Find more details about `jtreg`'s scratch directory at https://openjdk.org/jtreg/faq.html#scratch-directory
>> 
>> In addition to use the current working directory for `@TempDir` annotated paths, this change also switches off JUnit's built-in cleanup of temporary files - leaving it to `jtreg` to care about it, see for example the help message for  the `-retain` command-line option:
>> 
>> 
>>     -retain | -retain:<pass,fail,error,all,file-pattern>,...
>>                     Specify files to be retained after each test completes
>>                     executing. If -retain is not specified, only the files from
>>                     the last test executed will be retained. If -retain is
>>                     specified with no argument, all files will be retained.
>>                     Otherwise, the files may be described by one or more of the
>>                     following values:
>>         none            Do not retain any of the files generated by each test
>>         pass            Retain files generated by tests that pass
>>         fail            Retain files generated by tests that fail
>>         error           Retain files generated by tests that caused an error
>>         all             Retain all files generated by each test
>>         file-pattern    Retain files that match a specific filename. The name
>>                         may contain '*' to match any sequence of characters. For
>>                         example, result.* or *.err.
>
> test/junitTrace/JupiterTempDir.java line 39:
> 
>> 37: class JupiterTempDir {
>> 38:     @Test
>> 39:     @EnabledIfSystemProperty(named = "test.name", matches = "JupiterTempDir.java")
> 
> Hello Christian, why is this needed in this test?

The assertion(s) in the test only make sense, when `jtreg` runs the test via its `JUnitRunner`. The `test.name` system property is an indicator that `jtreg` runs the test, I could switch to another one documented here: https://openjdk.org/jtreg/tag-spec.html#testvars

> test/junitTrace/JupiterTempDir.java line 44:
> 
>> 42:         var expected = currentWorkingDirectory.toAbsolutePath().toString();
>> 43:         var actual = temporary.getParent().toAbsolutePath().toString();
>> 44:         Assertions.assertEquals(expected, actual, "Unexpected root for temporary files");
> 
> Would `Files.isSameFile(currentWorkingDirectory, temporary.getParent())` be more appropriate here?

Not sure if this would hold true for every case, especially for some fancy (sym-)linked directories?

I think the `String`-based comparison is good-enough here, and has a nice side-effect producing a human-readable error messages when the assertion fails.

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

PR Review Comment: https://git.openjdk.org/jtreg/pull/248#discussion_r1967454480
PR Review Comment: https://git.openjdk.org/jtreg/pull/248#discussion_r1967460618