Hello Christoph,
Thank you for the very detailed review. Comments inline.
On 24/10/19 3:00 AM, Langer, Christoph wrote:
>
> For the path to the test file, you could simply do: final Path jarPath =
> Paths.get("zipfs-crc-test.jar");
> The test is run in the scratch directory of jtreg, so no reason to go to
> java.io.tmpdir.
Thank you. I wasn't aware of jtreg scratch dir. Does it get used for
every test or is there something specific to the location/type of this
test? The updated webrev now uses the above construct.
> You can also skip deletion of this test file in the finally block, as the
> jtreg scratch directories will be wiped by jtreg eventually.
>
> For the existence check of the test file in line 62, you can simply use
> Files.exists.
Indeed. Updated in new webrev.
> As for creating the zipfs Filesystem (line 68), you can simply use
> FileSystems.newFileSystem(Path, Map), no need to mess around with URIs.
The FileSystems.newFileSystem(Path, Map) doesn't exist in Java 11[1]. Of
course, this specific test will be run against latest upstream, where
this new method exists. I actually copied over that usage from my Java
11 reproducer. But given that this issue isn't about creating the
FileSystem itself, I have taken your advice and changed this line in the
new webrev.
>
> Line 96, where construct the input stream and then in 97, the ZipInputStream,
> I suggest you either put both into the try statement or you use
> ZipInputStream zis = new ZipInputStream(Files.newInputStream(jarPath)) and
> then rely on ZipInputStream cascading the close.
Done - updated in new webrev.
>
> And my last suggestion: you could statically import Assert.assertEquals to
> shorten lines 105 and 106.
Done - updated in new webrev.
The updated webrev is here
https://cr.openjdk.java.net/~jpai/webrev/8232879/2/webrev/
[1]
https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/nio/file/FileSystems.html
-Jaikiran
