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

Reply via email to