8321616: Retire binary test vectors in test/jdk/java/util/zip/ZipFile #17038

eirbjo · 2023-12-08T20:28:20Z

This PR suggests we retire the binary test vectors ZipFile/input.zip, ZipFile/input.jar and ZipFile/crash.jar

Binary test vectors are harder to analyze, and sharing test vectors across unrelated tests increases maintenance burden. It would be better to have each test produce its own test vectors independently.

While visiting these dusty tests, we should take the opportunity to convert them to JUnit, add more comments and perform some mild modernization and cleanups where appropriate. We should also consider whether any test are duplicated and can be retired or moved into other test files as separate methods. See comments below.

To help reviewers, here are some comments on the updated tests:

Available.java
This test currently has no jtreg @test header, so isn't currently active. After discussion, we decided to merge this test into ReadZip.java. I added some checks to verify that reading from the stream reduces the number of available bytes accordingly, also checking the behavior when the stream is closed.

CopyJar.java
The concern of copying entries seems to now have better coverage in the test zip/CopyZipFile. We decided to retire this test rather than convert it and instead convert CopyZipFile to JUnit.

EnumAfterClose.java
To prevent confusion with Java Enums, I suggest we rename this test to EnumerateAfterClose.

FinalizeInflater.java
The code verified by this test has been updated to use cleaners instead of finalizers. Still, this test code relies on finalizers. Not sure if this is an issue, but this test will need to be updated when finalizers are finally removed.

GetDirEntry.java
We decided to merge this test into ReadZip.readDirectoryEntries rather than keeping it as a separate test.

ReadZip.java
Nothing exciting here, the single main method was split into multiple JUnit methods, each focusing on a separate concern. A new test case noentries() was added, resolving JDK-8322830

ReleaseInflater.java
Nothing exciting, tried to add some comment to help understanding of what is tested.

StreamZipEntriesTest.java
This test was using TestNG so was converted to JUnit for consistency. Added some comments to help understanding.

ReadAfterClose.java
This uses the binary test vector crash.jar. The test is converted to JUnit and moved to `ReadZip.readAfterClose´. The test is expanded to exercise more read methods.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issues

JDK-8321616: Retire binary test vectors in test/jdk/java/util/zip/ZipFile (Enhancement - P4)
JDK-8322830: Add test case for ZipFile opening a ZIP with no entries (Enhancement - P4)

Reviewers

Lance Andersen (@LanceAndersen - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/17038/head:pull/17038
$ git checkout pull/17038

Update a local copy of the PR:
$ git checkout pull/17038
$ git pull https://git.openjdk.org/jdk.git pull/17038/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17038

View PR using the GUI difftool:
$ git pr show -t 17038

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/17038.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2023-12-08T20:29:21Z

👋 Welcome back eirbjo! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-12-08T20:30:36Z

@eirbjo The following label will be automatically applied to this pull request:

core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2023-12-11T15:13:39Z

Webrevs

LanceAndersen · 2023-12-11T16:51:28Z

Thank you for tacking the conversion of this test to junit

One quick comment, if we are updating this test, we should look to get rid of input.zip which means we should also address as part of the change:

open/test/jdk/java/util/zip/ZipFile/EnumAfterClose.java
open/test/jdk/java/util/zip/ZipFile/FinalizeInflater.java
open/test/jdk/java/util/zip/ZipFile/StreamZipEntriesTest.java

eirbjo · 2023-12-11T17:22:14Z

One quick comment, if we are updating this test, we should look to get rid of input.zip

I started going down that road, but felt uneasy about the amount of unrelated changes in a single PR. I'd like to make efficient use of reviewer time, so my preference was to focus on the JUnit conversion, without too many other changes.

If you prefer that we get rid of input.zip and also convert the affected tests to JUnit in the same PR, then I'm happy to switch strategy.

Note that StreamZipEntriesTest also uses input.jar, this is also read by Available, CopyJar, GetDirEntry, ReleaseInflater. Should we get rid of input.jar as well, or perhaps defer that to a follow-up?

LanceAndersen · 2023-12-11T17:40:24Z

One quick comment, if we are updating this test, we should look to get rid of input.zip

I started going down that road, but felt uneasy about the amount of unrelated changes in a single PR. I'd like to make efficient use of reviewer time, so my preference was to focus on the JUnit conversion, without too many other changes.

If you prefer that we get rid of input.zip and also convert the affected tests to JUnit in the same PR, then I'm happy to switch strategy.

Note that StreamZipEntriesTest also uses input.jar, this is also read by Available, CopyJar, GetDirEntry, ReleaseInflater. Should we get rid of input.jar as well, or perhaps defer that to a follow-up?

input.zip and input.jar are pretty trivial:

jar tvf test/jdk/java/util/zip/ZipFile/input.zip
506 Fri May 28 16:32:31 EDT 1999 ReadZip.java
jar tvf test/jdk/java/util/zip/ZipFile/input.jar
0 Tue Mar 23 13:09:08 EST 1999 META-INF/
86 Tue Mar 23 13:09:08 EST 1999 META-INF/MANIFEST.MF
728 Tue Mar 23 13:08:30 EST 1999 ReleaseInflater.java

So you could have the tests create the zip files on the fly or store the contents in an array within the tests.

If the tests create the zip file/jar on the fly, then we just need to make sure that we create it in the same fashion as the test needs.

I just think that we should take this opportunity as part of re-writing the tests to remove the need for the binary files in the workspace.

I don't have a preference whether we deal with input.jar separately, but these tests are not that complex so I do not see a risk if they are all converted at once, or done piece meal

eirbjo · 2023-12-11T17:45:43Z

I don't have a preference whether we deal with input.jar separately, but these tests are not that complex so I do not see a risk if they are all converted at once, or done piece meal

Thanks, I'll extend the goal of this PR to remove input.jar, input.zip and convert affected tests to JUnit along the way.

…t.jar, converting affected tests to JUnit

…anifest, lets keep it that way

eirbjo · 2023-12-14T10:13:31Z

The scope of this PR has now expanded to removing uses of the input.zip and input.jar files, updating any test using them to produce their own test vectors, and convert affected tests to JUnit.

I'm marking the PR ready for review again. Before looking too closely at the code, it would be useful to discuss the following tests:

Available.java: This test has no jtreg header. I've added one and converted the test. Is this worthwhile, or should we rather remove it?
CopyJar.java: The concern tested seems to have superior coverage in the test zip/CopyZipFile.java. Should we retire CopyJar.java instead of coverting it?
DirEntry.java: There is duplication between this test and ReadZip.readDirectoryEntry(). Should we retire one of these?

LanceAndersen · 2023-12-14T11:39:22Z

The scope of this PR has now expanded to removing uses of the input.zip and input.jar files, updating any test using them to produce their own test vectors, and convert affected tests to JUnit.

I'm marking the PR ready for review again. Before looking too closely at the code, it would be useful to discuss the following tests:

Available.java: This test has no jtreg header. I've added one and converted the test. Is this worthwhile, or should we rather remove it?

This could be moved into ReadZip. I do not believe we have a specific test and it is trivial

CopyJar.java: The concern tested seems to have superior coverage in the test zip/CopyZipFile.java. Should we retire CopyJar.java instead of coverting it?

Yes CopyZipFile already exercises Zipfile.ZipInputStream so it is safe to retire CopyJar (though CopyZipFile could use a junit conversion ;-)

DirEntry.java: There is duplication between this test and ReadZip.readDirectoryEntry(). Should we retire one of these?

I believe you meant GetDirEntry.java not DirEntry.java?

Having a test that specifically validates we can read META-INF is not a bad thing, but I suspect we have a test that already does that if not in the java/util/zip tests or java/util/jar tests. If not we should keep it but merge it as you suggest

eirbjo · 2023-12-14T12:22:28Z

@LanceAndersen

Thanks for your guidance! I moved Available into ReadZip, deleted CopyJar and merged GetDirEntry into ReadZip.readDirectoryEntries (adding a 'META-INF/' directory just in case)

eirbjo · 2023-12-14T14:05:16Z

(though CopyZipFile could use a junit conversion ;-)

Agreed, I have included that conversion in this PR :-) This test can make good use of assertThrows and splitting different concerns into separate test methods.

…ze is calledExplicitly

eirbjo · 2023-12-14T15:00:15Z

Seeing that ReadAfterClose.java uses a binary test vector crash.jar, I think it makes sense to include it in this PR, convert it to JUnit and move it into ReadZip.readAfterClose.

This removes the last remaining binary test vector ZIP in the ZipFile/ directory.

LanceAndersen · 2023-12-15T11:15:30Z

Seeing that ReadAfterClose.java uses a binary test vector crash.jar, I think it makes sense to include it in this PR, convert it to JUnit and move it into ReadZip.readAfterClose.

This removes the last remaining binary test vector ZIP in the ZipFile/ directory.

These are on my list to review, thank you for the update.

One comment, for the tests that we are removing/retiring, we probably want to include the bug numbers and/or the name of the original test class in the new home for the tests to make it easier for future maintainers to chase back the history in the unlikely event that an issue arises...

eirbjo · 2024-01-02T13:22:26Z

/issue add 8322830

openjdk · 2024-01-02T13:23:00Z

@eirbjo
Adding additional issue to issue list: 8322830: Add test case for ZipFile opening a ZIP with no entries.

LanceAndersen

Changes look fine. Probably need to change the copyright from 2023 -> 2024

LanceAndersen · 2023-12-18T19:38:55Z

test/jdk/java/util/zip/ZipFile/ReadZip.java

+        // Create zip file with Zip64 end
+        URI uri = URI.create("jar:" + zip.toUri());
+        Map<String, Object> env = Map.of("create", "true", "forceZIP64End", "true");
+        try (FileSystem fs = FileSystems.newFileSystem(uri, env)) {


No Need to use a URI here

test/jdk/java/util/zip/ZipFile/ReadZip.java

LanceAndersen · 2023-12-18T19:40:14Z

test/jdk/java/util/zip/ZipFile/ReadZip.java

-        // ZIP64 end record
-        if (Files.notExists(Paths.get("/usr/bin/zip")))
+    /**
+     * Read a zip file created via "echo hello | zip dst.zip -",


This was created using info-zip so I would clarify in the comments

LanceAndersen · 2023-12-18T19:44:14Z

test/jdk/java/util/zip/ZipFile/ReadZip.java

+     */
+    @Test
+    public void readZip64EndZipProcess() throws IOException, InterruptedException {
+        if (Files.notExists(Paths.get("/usr/bin/zip"))) {


We should address this as the test won't run on Windows. It would be better to store the zip as a byte array so that it can be processed on all platforms and by removing ProcessBuilder, the test run will speed up a bit

Updated comment to reference Info-ZIP. Updated the code to read the ZIP from a hex-encoded string.

openjdk · 2024-01-08T17:32:20Z

@eirbjo This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8321616: Retire binary test vectors in test/jdk/java/util/zip/ZipFile
8322830: Add test case for ZipFile opening a ZIP with no entries

Reviewed-by: lancea

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 165 new commits pushed to the master branch:

e70cb4e: 8322565: (zipfs) Files.setPosixPermissions should preserve 'external file attributes' bits
d89602a: 8322982: CTW fails to build after 8308753
3bd9042: 8320788: The system properties page is missing some properties
525063b: 8322878: Including sealing information Class.toGenericString()
c1282b5: 8323540: assert((!((((method)->is_trace_flag_set(((1 << 4) << 8))))))) failed: invariant
5ba69e1: 8322477: order of subclasses in the permits clause can differ between compilations
c96cbe4: 8313083: Print 'rss' and 'cache' as part of the container information
a7db4fe: 8323428: Shenandoah: Unused memory in regions compacted during a full GC should be mangled
b86c3b7: 8309218: java/util/concurrent/locks/Lock/OOMEInAQS.java still times out with ZGC, Generational ZGC, and SerialGC
475306b: 7057369: (fs spec) FileStore getUsableSpace and getUnallocatedSpace could be clearer
... and 155 more: https://git.openjdk.org/jdk/compare/e2042421187dafc1aea75ffe15caf8beb824205b...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

eirbjo · 2024-01-09T07:24:00Z

Changes look fine. Probably need to change the copyright from 2023 -> 2024

No IP was produced in 2024 for most of these files, but I've updated them anyhow.

eirbjo · 2024-01-09T07:26:37Z

Thanks for your review, @LanceAndersen!

You might want to take a quick look at the lastest updates addressing your review before I integrate.

LanceAndersen · 2024-01-10T21:27:55Z

Thanks for your review, @LanceAndersen!

You might want to take a quick look at the lastest updates addressing your review before I integrate.

Looks good. I am running mach5 tiers1-3 internally once it completes, I will approve again

LanceAndersen

Internal mach5 tiers 1-3 completed

eirbjo · 2024-01-11T06:32:00Z

/integrate

openjdk · 2024-01-11T06:32:28Z

Going to push as commit 26de9e2.
Since your change was applied there have been 166 commits pushed to the master branch:

b530c02: 8317804: com/sun/jdi/JdwpAllowTest.java fails on Alpine 3.17 / 3.18
e70cb4e: 8322565: (zipfs) Files.setPosixPermissions should preserve 'external file attributes' bits
d89602a: 8322982: CTW fails to build after 8308753
3bd9042: 8320788: The system properties page is missing some properties
525063b: 8322878: Including sealing information Class.toGenericString()
c1282b5: 8323540: assert((!((((method)->is_trace_flag_set(((1 << 4) << 8))))))) failed: invariant
5ba69e1: 8322477: order of subclasses in the permits clause can differ between compilations
c96cbe4: 8313083: Print 'rss' and 'cache' as part of the container information
a7db4fe: 8323428: Shenandoah: Unused memory in regions compacted during a full GC should be mangled
b86c3b7: 8309218: java/util/concurrent/locks/Lock/OOMEInAQS.java still times out with ZGC, Generational ZGC, and SerialGC
... and 156 more: https://git.openjdk.org/jdk/compare/e2042421187dafc1aea75ffe15caf8beb824205b...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-01-11T06:32:34Z

@eirbjo Pushed as commit 26de9e2.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

eirbjo added 2 commits December 8, 2023 21:21

Convert ReadZip to JUnit

9521c1e

Update copyright year

f6a00f0

openjdk bot added the core-libs core-libs-dev@openjdk.org label Dec 8, 2023

eirbjo marked this pull request as ready for review December 11, 2023 15:09

openjdk bot added the rfr Pull request is ready for review label Dec 11, 2023

eirbjo marked this pull request as draft December 12, 2023 19:29

openjdk bot removed the rfr Pull request is ready for review label Dec 12, 2023

Remove the use of binary test vector ZIP/JAR files input.zip and inpu…

57722b6

…t.jar, converting affected tests to JUnit

eirbjo changed the title ~~8321616: Convert the ReadZip test to JUnit~~ 8321616: Remove binary test vectors ZipFile/input.jar and ZipFile/input.zip Dec 13, 2023

eirbjo changed the title ~~8321616: Remove binary test vectors ZipFile/input.jar and ZipFile/input.zip~~ 8321616: Retire binary test vectors ZipFile/input.jar and ZipFile/input.zip Dec 13, 2023

eirbjo added 3 commits December 13, 2023 12:02

The input.jar test vector included a META-INF/ directory before the m…

bafd545

…anifest, lets keep it that way

Merge branch 'master' into readzip-junit

5e3247a

Add @bug reference 4401122 on the Available.java test

56f55d8

eirbjo marked this pull request as ready for review December 14, 2023 10:13

openjdk bot added the rfr Pull request is ready for review label Dec 14, 2023

eirbjo added 3 commits December 14, 2023 13:04

Merge Available.java into ReadZip.java

c0f1d97

Retire redundant test ZipFile/CopyJar

78b33cb

Merge GetDirEntry.java into ReadZip.readDirectoryEntries()

03cb835

openjdk bot removed the rfr Pull request is ready for review label Dec 14, 2023

Remove trailing whitespace

593a76c

openjdk bot added the rfr Pull request is ready for review label Dec 14, 2023

Convert CopyZipFile.java to JUnit

4e9c97d

eirbjo added 2 commits December 14, 2023 15:26

Improve comment explaining what happens when ZipEntry.setCompressedSi…

8623eff

…ze is calledExplicitly

Merge the ReadAfterClose test into ReadZip, converting it to JUnit.

27fe113

eirbjo changed the title ~~8321616: Retire binary test vectors ZipFile/input.jar and ZipFile/input.zip~~ 8321616: Retire binary test vectors in test/jdk/java/util/zip/ZipFile Dec 14, 2023

eirbjo added 2 commits December 20, 2023 22:12

Merge branch 'master' into readzip-junit

85500cd

Add test verifying that ZipFile can open a ZIP file with zero entries

669b8da

LanceAndersen approved these changes Jan 8, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 8, 2024

eirbjo added 2 commits January 9, 2024 07:57

Address review comments for ZIP64 End Of Central header tests

c762f4a

LanceAndersen approved these changes Jan 10, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jan 11, 2024

openjdk bot closed this Jan 11, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8321616: Retire binary test vectors in test/jdk/java/util/zip/ZipFile #17038

8321616: Retire binary test vectors in test/jdk/java/util/zip/ZipFile #17038

eirbjo commented Dec 8, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Dec 8, 2023

openjdk bot commented Dec 8, 2023

mlbridge bot commented Dec 11, 2023 •

edited

Loading

LanceAndersen commented Dec 11, 2023

eirbjo commented Dec 11, 2023

LanceAndersen commented Dec 11, 2023

eirbjo commented Dec 11, 2023

eirbjo commented Dec 14, 2023 •

edited

Loading

LanceAndersen commented Dec 14, 2023

eirbjo commented Dec 14, 2023

eirbjo commented Dec 14, 2023

eirbjo commented Dec 14, 2023

LanceAndersen commented Dec 15, 2023

eirbjo commented Jan 2, 2024

openjdk bot commented Jan 2, 2024

LanceAndersen left a comment

LanceAndersen Dec 18, 2023

eirbjo Jan 9, 2024

LanceAndersen Dec 18, 2023

LanceAndersen Dec 18, 2023

eirbjo Jan 9, 2024

openjdk bot commented Jan 8, 2024 •

edited

Loading

eirbjo commented Jan 9, 2024

eirbjo commented Jan 9, 2024

LanceAndersen commented Jan 10, 2024

LanceAndersen left a comment

eirbjo commented Jan 11, 2024

openjdk bot commented Jan 11, 2024

openjdk bot commented Jan 11, 2024

8321616: Retire binary test vectors in test/jdk/java/util/zip/ZipFile #17038

8321616: Retire binary test vectors in test/jdk/java/util/zip/ZipFile #17038

Conversation

eirbjo commented Dec 8, 2023 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Dec 8, 2023

openjdk bot commented Dec 8, 2023

mlbridge bot commented Dec 11, 2023 • edited Loading

Webrevs

LanceAndersen commented Dec 11, 2023

eirbjo commented Dec 11, 2023

LanceAndersen commented Dec 11, 2023

eirbjo commented Dec 11, 2023

eirbjo commented Dec 14, 2023 • edited Loading

LanceAndersen commented Dec 14, 2023

eirbjo commented Dec 14, 2023

eirbjo commented Dec 14, 2023

eirbjo commented Dec 14, 2023

LanceAndersen commented Dec 15, 2023

eirbjo commented Jan 2, 2024

openjdk bot commented Jan 2, 2024

LanceAndersen left a comment

Choose a reason for hiding this comment

LanceAndersen Dec 18, 2023

Choose a reason for hiding this comment

eirbjo Jan 9, 2024

Choose a reason for hiding this comment

LanceAndersen Dec 18, 2023

Choose a reason for hiding this comment

LanceAndersen Dec 18, 2023

Choose a reason for hiding this comment

eirbjo Jan 9, 2024

Choose a reason for hiding this comment

openjdk bot commented Jan 8, 2024 • edited Loading

eirbjo commented Jan 9, 2024

eirbjo commented Jan 9, 2024

LanceAndersen commented Jan 10, 2024

LanceAndersen left a comment

Choose a reason for hiding this comment

eirbjo commented Jan 11, 2024

openjdk bot commented Jan 11, 2024

openjdk bot commented Jan 11, 2024

eirbjo commented Dec 8, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Dec 11, 2023 •

edited

Loading

eirbjo commented Dec 14, 2023 •

edited

Loading

openjdk bot commented Jan 8, 2024 •

edited

Loading