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` This pull request has now been integrated. Changeset: b6700095 Author: Eirik Bjørsnøs <eir...@openjdk.org> URL: https://git.openjdk.org/jdk/commit/b6700095c018a67a55b746cd4eee763c68f538e0 Stats: 116 lines in 1 file changed: 0 ins; 116 del; 0 mod 8338729: Retire the test jdk/java/util/zip/TestZipError.java Reviewed-by: lancea ------------- PR: https://git.openjdk.org/jdk/pull/20660
