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 I support rewriting tests like this into junit 5. I wrote many jdk tests (like this) before a standard high quality test framework like junit 5 was available. Zip file implementations are 90% corner cases, so expanding testing and improving testability of jdk classes would be valuable. For example, it would be nice to explicitly request use of ZIP64 extensions even when they are not needed, to avoid having to create multi-gigabyte test files. test/jdk/java/util/zip/ZipFile/CorruptedZipFiles.java line 246: > 244: /* > 245: * Validate that a ZipException is thrown if the last CEN header is > 246: * not immediatly followed by the start of the 'End of central > directory' record typo: immediatly test/jdk/java/util/zip/ZipFile/CorruptedZipFiles.java line 300: > 298: > 299: /* > 300: * Validate that a ZipException is thrown when a LOC header has an > unexpected signature I would leave off "Validate that" ------------- PR Review: https://git.openjdk.org/jdk/pull/12563#pullrequestreview-1367398354 PR Review Comment: https://git.openjdk.org/jdk/pull/12563#discussion_r1154773604 PR Review Comment: https://git.openjdk.org/jdk/pull/12563#discussion_r1154779308
