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". Other than that, it's good to go for mw. 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? 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> > >
