RFR: JDK-8232879: (zipfs) Writing out data with ZipFileSystem leads to a CRC failure in the generated jar file

[Jaikiran Pai]

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