On Wed, 22 Apr 2026 08:00:17 GMT, SendaoYan <[email protected]> wrote:
>> Jaikiran Pai has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - remove toAbsolutePath() usage in test
>> - add code comments in test to explain why we use /tmp/ directory for
>> testExtractNoDestDirWithPFlag
>
> test/jdk/tools/jar/JarExtractTest.java line 71:
>
>> 69: // the jar that will get extracted in the tests
>> 70: private Path testJarPath;
>> 71: private static Collection<Path> filesToDelete = new ArrayList<>();
>
> filesToDelete can set as final
Fixed in the updated PR.
> test/jdk/tools/jar/JarExtractTest.java line 302:
>
>> 300: } catch (IOException ioe) {
>> 301: Assumptions.abort("skipping test, since /tmp cannot be
>> written to: " + ioe);
>> 302: return;
>
> This return sentence seems unreachable,
The `Assumptions.abort(...)` does indeed throw an exception, but I prefer to
retain this `return` because I find it clearer when reading the code. If you
have a strong preference, let me know and I can remove it.
> test/jdk/tools/jar/JarExtractTest.java line 532:
>
>> 530: }
>> 531:
>> 532: private static void deleteRecursively(final Path dir) throws
>> IOException {
>
> Can we use the existing
> test/lib/jdk/test/lib/util/FileUtils.deleteFileTreeWithRetry(Path) instead of
> create new one.
Hello @sendaoYan, I had considered that, but that utility class loads a test
native library. Given that this method in the test doesn't have any complex
code, I decided to just have this local to the test, instead of bringing in the
FileUtils class.
> test/jdk/tools/jar/JarExtractTest.java line 553:
>
>> 551: });
>> 552: }
>> 553: }
>
> Seems missing newline at end of file.
Fixed in the updated PR.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/30847#discussion_r3122985384
PR Review Comment: https://git.openjdk.org/jdk/pull/30847#discussion_r3122997258
PR Review Comment: https://git.openjdk.org/jdk/pull/30847#discussion_r3122977764
PR Review Comment: https://git.openjdk.org/jdk/pull/30847#discussion_r3122978818
