On Wed, 21 Aug 2024 09:59:38 GMT, Eirik Bjørsnøs <eir...@openjdk.org> wrote:
> Please review this test-only, cleanup PR which proposes that we retire the > `jdk/java/util/zip/TestZipError.java` test. > > The test has fallen out of sync with its original and stated purpose > described by its name, `@summary` tag and code comments. The error condition > actually being exercised has existing coverage in the `ZipSourceCache` test. > > Background: > > * The test was initially added in JDK-4615343 (Java 6) to exercise code paths > which caused `ZipFile` entry enumeration to throw a `ZipError` (a subclass of > `InternalError`). > * After the rewrite of the ZipFile API from native to Java in JDK-8145260 > (JDK 9), the Java code now tested no longer throws `ZipError` (The `ZipError` > class is no longer used in OpenJDK) > * As part of JDK-8145260, `TestZipError.java` was updated to catch > `ZipException` instead of `ZipError` for the expected/passing case. > Interestingly, a line was added which caused the test to not simply enumerate > the entries of the updated ZipFile, but to also consume the input stream of > the entries. > * The added line caused `ZipFileInputStream.initDataOffset` to throw a > `ZipException` when the CEN local header offset does not contain the expected > LOC header, resulting in a ZipException with the message "ZipFile invalid LOC > header (bad signature)". This added line caused the test to (accidentally?) > test something which is actually different than its original purpose. > * The "invalid LOC header" scenario is currently better tested by the > `ZipSourceCache` JUnit test, which also runs on non-Windows platforms. > * Note that JDK-8145260 did not update the copyright years in the license > header of this file. > > Considering all of the above, I suggest that we do not invest resources in > bringing the naming, summary and code comments of this test in line with what > is actually being tested. Instead, we should simply retire it in favor of the > existing `ZipSourceCache` test. > > Testing: > > * No tests are updated in this PR, I added a `noreg-cleanup` label in the JBS > * By collecting a stack trace, I verified that the `ZipException` caught by > `TestZipError` is indeed caused by the LOC header validation logic in > `ZipFileInputStream.initDataOffset` > * Similarly, I also verified that `ZipSourceCache` throws on the same > condition as `TestZipError` Thanks for catching this, I agree this test can go the way of the dodo bird. Not sure why it was kept (or at least re-worked/named) after [JDK-8145260](https://bugs.openjdk.org/browse/JDK-8145260) but I am sure there was a good reason at the time ------------- Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20660#pullrequestreview-2266492192
