On Fri, 31 Mar 2023 13:51:22 GMT, Eirik Bjorsnos <d...@openjdk.org> wrote:
>> CorruptedZipFiles could benefit from some spring cleaning and a conversion
>> to junit:
>>
>> - The actual tests are moved into their own `@Test` methods, given more
>> meaningful names and a Javadoc comment explaining the constraint being
>> verified
>> - The setup code is moved to a `@Before` method, slightly modernized and
>> rewritten to take advantage of `assertEquals`
>> - `checkZipExceptionImpl` is updated to take advantage of `assertThrows`
>> - A bunch of constants copied over from `ZipFile` can be deleted since
>> JDK-6225935 has long been fixed
>
> Eirik Bjorsnos has updated the pull request incrementally with four
> additional commits since the last revision:
>
> - Document the structure of the 'good' ZIP
> - Use the "Validate that.." comment style
> - Document the ByteBuffer with a comment
> - Use ByteBuffer when manipulating multi-byte fields
Thanks for your recent updates Eirik, I think this looks good.
A few minor comment suggestions
I will get a Mach 5 run done as a sanity check
test/jdk/java/util/zip/ZipFile/CorruptedZipFiles.java line 56:
> 54: * Byte array holding a valid template ZIP.
> 55: *
> 56: * This 'good' ZIP file has the following structure:
Change this -> the
test/jdk/java/util/zip/ZipFile/CorruptedZipFiles.java line 192:
> 190: /*
> 191: * Validate that a ZipException is thrown when the 'End of Central
> Directory'
> 192: * (END) header has a CEN offset incoherent with the position
> calculated
Not sure _incoherent_ is the best term.
perhaps something along the lines:
header contains an invalid CEN Directory starting offset.....
test/jdk/java/util/zip/ZipFile/CorruptedZipFiles.java line 203:
> 201: /*
> 202: * Validate that a ZipException is thrown when a A CEN header has an
> unexpected signature
> 203: */
change "a A" to "a"
-------------
PR Review: https://git.openjdk.org/jdk/pull/12563#pullrequestreview-1367326652
PR Review Comment: https://git.openjdk.org/jdk/pull/12563#discussion_r1154726957
PR Review Comment: https://git.openjdk.org/jdk/pull/12563#discussion_r1154735421
PR Review Comment: https://git.openjdk.org/jdk/pull/12563#discussion_r1154735787
