On Mon, 19 May 2025 12:58:00 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> David Beaumont has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fixed test.
>
> test/jdk/jdk/nio/zipfs/NewFileSystemTests.java line 239:
>
>> 237: assertTrue(fs.isReadOnly());
>> 238: if (!"First
>> version".equals(Files.readString(fs.getPath("file.txt"), UTF_8))) {
>> 239: fail("unexpected file content");
>
> Nit, for this and the other new `fail` statements, it might be good to
> include the unexpected file content in the fail message. If at all the test
> fails for whatever reason, that additional detail usually help to quickly
> debug the failures. Or maybe just replace the `if` followed by a `fail` with
> an `assertEquals()` call.
Urgh, my bad. That's the hangover from these tests being first implemented in
the ZipFSTester class without access to sensible assert methods.
> test/jdk/jdk/nio/zipfs/TestPosix.java line 2:
>
>> 1: /*
>> 2: * Copyright (c) 2019, 2025, SAP SE. All rights reserved.
>
> For non-Oracle copyright lines like these, we don't edit them and instead we
> introduce a newline for the Oracle copyright year. So we should revert the
> change to this line and introduce the following new line just after this
> current one:
>
> * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved.
Thanks! Done.
> test/jdk/jdk/nio/zipfs/Utils.java line 48:
>
>> 46: *
>> 47: * @param name the file name of the jar file to create in the
>> working directory.
>> 48: * @param entries a list of JAR entries to be populated with random
>> bytes.
>
> Nit - maybe reword this to:
>
>> @param entries JAR file entry names, whose content will be populated with
>> random bytes.
Done.
> test/jdk/jdk/nio/zipfs/Utils.java line 52:
>
>> 50: */
>> 51: static Path createJarFile(String name, String... entries) throws
>> IOException {
>> 52: Path jarFile = Paths.get(name);
>
> In recent times we have replaced `Paths.get(...)` call in the JDK code with
> `Path.of(...)`. We should do the same here and the other line in this utility
> class.
Good spot, thanks.
Sadly I'm currently working on other code in which Path.of() is not permitted,
so I won't naturally spot these things.
> test/jdk/jdk/nio/zipfs/Utils.java line 78:
>
>> 76: *
>> 77: * @param name the file name of the jar file to create in the
>> working directory.
>> 78: * @param entries a map of relative file name path strings to file
>> content
>
> Nit - I think we should replace this description with something like:
>
>> @param entries a map of JAR file entry names to entry content
Done.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095730823
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095740846
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095740347
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095735890
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095738222
