RFR: 8338729: Retire the test jdk/java/util/zip/TestZipError.java

Eirik Bjørsnøs eirbjo at openjdk.org
Fri Aug 23 14:00:13 UTC 2024


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`

-------------

Commit messages:
 - Retire the test TestZipError.java

Changes: https://git.openjdk.org/jdk/pull/20660/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20660&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8338729
  Stats: 116 lines in 1 file changed: 0 ins; 116 del; 0 mod
  Patch: https://git.openjdk.org/jdk/pull/20660.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/20660/head:pull/20660

PR: https://git.openjdk.org/jdk/pull/20660


More information about the core-libs-dev mailing list