Hi Christoph, > On Oct 28, 2019, at 6:19 AM, Langer, Christoph <christoph.lan...@sap.com> > wrote: > > Hi Lance, > > thanks for reworking the test. That definitely improves coverage. > > The comment for the test method (line 56/57) could be improved like: "Verify > all write methods of an OutputStream that can be used to write to an entry of > Zip Filesystem by exercising all of them when creating an archive and then > traversing the archive with ZipFile, ZipInputStream and the Zip Filesystem”.
I think I am going to leave it as it is for now. > > Other than that, it's good to go for mw. OK, thank you. > > BTW, I think meanwhile all these zipfs tests have quite some duplicate > infrastructure classes and methods (e.g. Entry) that could be shared. Maybe > we should try to carve out some stuff into a library or some Auxillary helper > class. What do you think? Yes it is on my todo list. There is an issue that needs addressed 1st then that will follow on. Best Lance > > Thanks > Christoph > >> -----Original Message----- >> From: core-libs-dev <core-libs-dev-boun...@openjdk.java.net> On Behalf >> Of Lance Andersen >> Sent: Samstag, 26. Oktober 2019 04:42 >> To: Jaikiran Pai <jai.forums2...@gmail.com> >> Cc: core-libs-dev@openjdk.java.net >> Subject: Re: RFR: JDK-8232879: (zipfs) Writing out data with ZipFileSystem >> leads to a CRC failure in the generated jar file >> >> >>> On Oct 25, 2019, at 10:10 PM, Jaikiran Pai <jai.forums2...@gmail.com> >> wrote: >>> >>> Thank you for the review, Lance. >>> >> >> You are very welcome Jaikiran. >>> On 26/10/19 4:25 AM, Lance Andersen wrote: >>>> >>>> The change to Zip FS looks good. I re-worked the test so that it also runs >> against ZipFile (which uses the CEN vs the LOC) and Zip FS in addition to >> ZipInputStream for extra coverage. >>>> >>>> The webrev can be found at: >> http://cr.openjdk.java.net/~lancea/8232879/webrev.00/index.html >> <http://cr.openjdk.java.net/~lancea/8232879/webrev.00/index.html>Looks >> fine. A very minor note about >> http://cr.openjdk.java.net/~lancea/8232879/webrev.00/test/jdk/jdk/nio/zip >> fs/CRCWriteTest.java.html >> <http://cr.openjdk.java.net/~lancea/8232879/webrev.00/test/jdk/jdk/nio/zi >> pfs/CRCWriteTest.java.html> >>> 136 while ((ze = zis.getNextEntry()) != null) { >>> 137 assertNotNull(ze); >>> >>> Looks like an oversight - that assertNotNull(ze) on 137 isn't needed due to >> the != null check on 1 >> >> Yep, not needed. Sometimes you do things on auto pilot ;-) >> >> Will remove before I push the change. >>> 36. >>> >>> -Jaikiran >>> >> >> <http://oracle.com/us/design/oracle-email-sig-198324.gif> >> <http://oracle.com/us/design/oracle-email-sig-198324.gif> >> <http://oracle.com/us/design/oracle-email-sig-198324.gif> >> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| >> Principal Member of Technical Staff | +1.781.442.2037 >> Oracle Java Engineering >> 1 Network Drive >> Burlington, MA 01803 >> lance.ander...@oracle.com <mailto:lance.ander...@oracle.com> >> >> > <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>
