8304014: Convert test/jdk/java/util/zip/ZipFile/CorruptedZipFiles.java to junit by eirbjo · Pull Request #12563 · openjdk/jdk

eirbjo · 2023-02-14T17:46:21Z

CorruptedZipFiles could benefit from some spring cleaning and a conversion to junit:

The actual tests are moved into their own @Test methods, given more meaningful names and a Javadoc comment explaining the constraint being verified
The setup code is moved to a @Before method, slightly modernized and rewritten to take advantage of assertEquals
checkZipExceptionImpl is updated to take advantage of assertThrows
A bunch of constants copied over from ZipFile can be deleted since JDK-6225935 has long been fixed

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

Issue

JDK-8304014: Convert test/jdk/java/util/zip/ZipFile/CorruptedZipFiles.java to junit

Reviewers

Lance Andersen (@LanceAndersen - Reviewer) ⚠️ Review applies to 3b6d8577

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12563

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

Using diff file

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

… copies which are now obsolete.

bridgekeeper · 2023-02-14T17:47:28Z

👋 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-02-14T17:48:57Z

@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.

LanceAndersen

Hi Eirik,

Thank you for your suggested changes to this test.

I think if we are going to re-work this test, we should go further including improving the comments for future maintainers

I made a quick pass and some initial thoughts are below

Best
Lance

LanceAndersen · 2023-02-27T20:47:42Z

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

+    private byte[] good;
+    // Copy of the template for modification in tests
+    private byte[] bad;
+    // Some well-known locations in the golden ZIP


Would be a good opportunity to provide better naming

I went for "template" and "copy" here.

LanceAndersen · 2023-02-27T20:49:07Z

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

+
+    @BeforeTest
+    public void setup() throws IOException {
+        // Make a ZIP with a single entry


Please change either this method name or the other setup() method.

I would also add a Files.delete(zip) here

Renamed the @BeforeMethod one to makeCopy

LanceAndersen · 2023-02-27T20:50:22Z

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

+        // Read the contents of the file
+        good = Files.readAllBytes(zip);
+        Files.delete(zip);
+


I would add a cleanup method in addition to deleting earlier on

This one I did not fully understand. In any case, there is really no need to write the template ZIP to disk, so I opted for a ByteArrayOutputStream instead. I added a cleanup method to delete the ZIP file produced by each test.

LanceAndersen · 2023-02-27T20:51:10Z

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

+        good = Files.readAllBytes(zip);
+        Files.delete(zip);
+
+        // Set up some well-known offsets


please expand the comments

LanceAndersen · 2023-02-27T20:52:14Z

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

+     * Make a copy safe to modify by each test
+     */
+    @BeforeMethod
+    public void setUp() {


As mentioned above, please rename this method or the other

Yes, renamed to makeCopy

LanceAndersen · 2023-02-27T20:52:47Z

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

+    }
+
+    @Test
+    public void corruptedENDSIZ() {


Please add a comment to this and the other tests describing the purpose of the test

I have given each @Test method a more meaningful name and added comments explaining the constraint or error condition being tested.

LanceAndersen · 2023-02-27T21:04:20Z

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

+            assertTrue(ex.getMessage().matches(msgPattern),
+                    "Unexpected ZipException message: " + ex.getMessage());
+
+        } catch (IOException e) {


Another option is to throw the IOException and add IOException to the test method signature as we are going to fail anyways for the IOException

Changed to propagate the IOException

LanceAndersen · 2023-02-27T21:12:02Z

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

    }
-
    static int uniquifier = 432;



I am not sure how many people would know what a uniquifier is though it is an interesting phrase which I know from SQL Server. Perhaps consider renaming this variable

I'm not sure we need unique file names for this test. Opted for a single ZIP Path instead and removed this variable.

…nd what these tests verify.

eirbjo · 2023-02-28T09:00:24Z

Hopefully this test is now easier to understand for someone who is not intimately familiar with the ZIP format. I am hearing a voice in the back of my head (Martin?) saying "this level of documentation is excessive" though. As always a trade off I guess.

eirbjo · 2023-03-11T08:06:21Z

/issue 8304014

openjdk · 2023-03-11T08:08:07Z

@eirbjo The primary solved issue for a PR is set through the PR title. Since the current title does not contain an issue reference, it will now be updated.

mlbridge · 2023-03-29T09:32:19Z

Webrevs

…r. Collapse the checkZipException and checkZipExceptionInGetInputStream into one method by always consuming the InputStream. Add a block comment for the assertZipException method.

dfuch · 2023-03-29T13:23:19Z

High level comment: these days we usually try to use junit 5 / jupiter instead of TestNG.

eirbjo · 2023-03-29T13:30:21Z

High level comment: these days we usually try to use junit 5 / jupiter instead of TestNG.

I think my choice of testNG here might have been influenced by other ZIP area tests using testNG. I guess a rewrite to junit should be pretty straightforward.

@LanceAndersen Do you have any opinion on junit/testNG for tests like this?

eirbjo · 2023-03-29T14:01:42Z

@LanceAndersen Do you have any opinion on junit/testNG for tests like this?

Here's a junit version for consideration:

https://github.com/eirbjo/jdk/blob/corrupted-zip-files-ng-junit/test/jdk/java/util/zip/ZipFile/CorruptedZipFiles.java

LanceAndersen · 2023-03-29T15:21:04Z

High level comment: these days we usually try to use junit 5 / jupiter instead of TestNG.

I think my choice of testNG here might have been influenced by other ZIP area tests using testNG. I guess a rewrite to junit should be pretty straightforward.

@LanceAndersen Do you have any opinion on junit/testNG for tests like this?

We have had some discussion about using junit vs testNG but we have not mandated it so I am OK with either but certainly if you would like to move to junit as it should be fairly straight forward for this test, I think that would be nice

LanceAndersen · 2023-03-29T15:24:41Z

@LanceAndersen Do you have any opinion on junit/testNG for tests like this?

Here's a junit version for consideration:

https://github.com/eirbjo/jdk/blob/corrupted-zip-files-ng-junit/test/jdk/java/util/zip/ZipFile/CorruptedZipFiles.java

Let's go with reviewing this version, Thank you for the update Eirik

eirbjo · 2023-03-29T18:09:06Z

Let's go with reviewing this version, Thank you for the update Eirik

Good, I pushed the junit version to the PR. Also updated the JBS and PR titles.

Big thanks to @dfuch for suggesting, your lower-level comments are also welcome!

dfuch

Hi Eirik, thanks for following up on my suggestion :-)

dfuch · 2023-03-29T18:25:34Z

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

+import org.junit.After;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+


I believe you should be using corresponding annotations from org.junit.jupiter.api instead.
From https://junit.org/junit5/docs/current/user-guide/index.html#migrating-from-junit4

@Before and @After no longer exist; use @BeforeEach and @AfterEach instead.
@BeforeClass and @AfterClass no longer exist; use @BeforeAll and @AfterAll instead.

Similarly the @Test annotation should be imported from org.junit.jupiter.api

Noob question: Do you know where I find the jars for setting up junit5 in my IDE? jtreg's junit-platform-console-standalone-1.9.2 does not seem to include these annotations.

Seems my dev environment has jtreg < 7. Maybe that's a problem?

Seems junit 5 support is a recent effort?

https://mail.openjdk.org/pipermail/jdk-dev/2022-August/006869.html

Scratch that. It seems my IDE was just not being very cooperative in suggesting or finding JUnit 5 API. When I wrote the imports myself it seems to have improved its behaviour.

Do you think it looks better now, @dfuch ?

Yes - this a recent effort. I believe you will need at least jtreg 7, and JUnit 5.8.2

It is worth noting that I did a new local build of jtreg and that may have updated the junit API jar.

I would find that weird because the file name or version of the JAR file was not updated jtreg/build/deps/junit/junit-platform-console-standalone-1.9.2.jar. But maybe the content of the file changed?

eirbjo · 2023-03-31T13:51:35Z

Following an offline discussion with Lance, I added the following improvements:

Use a little-endian ByteBuffer to manipulate multi-byte fields (Manipulating single bytes seemed too 'magic')
Document the structure of the 'good' ZIP by including the zipdetails. (Maintainers should not be expected to know every detail of the ZIP format)
Use the 'Validate that a ZipException is thrown..' comment style consistently (Easier to read)

eirbjo · 2023-03-31T13:58:01Z

@Martin-Buchholz this PR suggests we can improve the CorruptedZipFiles test.

Would be interested in hearing if you agree this is an improvement? If not, why?

Thanks!

LanceAndersen

Thanks for your recent updates Eirik, I think this looks good.

A few minor comment suggestions

I will get a Mach 5 run done as a sanity check

LanceAndersen · 2023-03-31T17:27:25Z

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

+    /*
+     * Byte array holding a valid template ZIP.
+     *
+     * This 'good' ZIP file has the following structure:


Change this -> the

LanceAndersen · 2023-03-31T17:38:02Z

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

-        return data[offset]&0xff;
+    /*
+     * Validate that a ZipException is thrown when the 'End of Central Directory'
+     * (END) header has a CEN offset incoherent with the position calculated


Not sure incoherent is the best term.

perhaps something along the lines:

header contains an invalid CEN Directory starting offset.....

I remember my struggles trying to write this one in the first place. The code says:

long cenpos = end.endpos - end.cenlen; // position of CEN table // Get position of first local file (LOC) header, taking into // account that there may be a stub prefixed to the zip file. locpos = cenpos - end.cenoff; if (locpos < 0) { zerror("invalid END header (bad central directory offset)"); }

so I ended up trying to express this equation in words, which is maybe not so successful. I don't see incoherent as the main problem, if it was we could just use inconsistent.

Do you have a better concrete suggestion?

I would keep it simple and indicate that the offset of start of central directory field within the End Header was given an invalid value

LanceAndersen · 2023-03-31T17:38:26Z

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

-        return u8(data,offset) + (u8(data,offset+1)<<8);
+    /*
+     * Validate that a ZipException is thrown when a A CEN header has an unexpected signature
+     */


change "a A" to "a"

Martin-Buchholz

I support rewriting tests like this into junit 5.

I wrote many jdk tests (like this) before a standard high quality test framework like junit 5 was available.

Zip file implementations are 90% corner cases, so expanding testing and improving testability of jdk classes would be valuable. For example, it would be nice to explicitly request use of ZIP64 extensions even when they are not needed, to avoid having to create multi-gigabyte test files.

Martin-Buchholz · 2023-03-31T18:24:26Z

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

-    static final int LOCEXT = ZipFile.LOCEXT;
+    /*
+     * Validate that a ZipException is thrown if the last CEN header is
+     * not immediatly followed by the start of the 'End of central directory' record


typo: immediatly

Thanks, fixed.

Martin-Buchholz · 2023-03-31T18:31:37Z

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

+    }
+
+    /*
+     * Validate that a ZipException is thrown when a LOC header has an unexpected signature


I would leave off "Validate that"

Thanks, updated all test comments to strip this shared prefix.

eirbjo · 2023-03-31T20:07:55Z

I support rewriting tests like this into junit 5.

Glad to hear! I would be a bit sad to hear you were against this.

I wrote many jdk tests (like this) before a standard high quality test framework like junit 5 was available.

And thank you for that!

Zip file implementations are 90% corner cases, so expanding testing and improving testability of jdk classes would be valuable. For example, it would be nice to explicitly request use of ZIP64 extensions even when they are not needed, to avoid having to create multi-gigabyte test files.

Indeed. I would also just love to be able to transform existing ZIP files into Zip64 format, regardless of their current format. And along the way transform any ZIP record or field in a type-safe manner. Stay tuned for suggestions in this area..

LanceAndersen

Last Mach5 runs continue to be clean so you are good to integrate and then I can sponsor Monday

Thank you for your efforts on this clean up Eirik.

openjdk · 2023-04-02T17:46:39Z

@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:

8304014: Convert test/jdk/java/util/zip/ZipFile/CorruptedZipFiles.java to junit

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 64 new commits pushed to the master branch:

127afd3: 8241613: Suspicious calls to MacroAssembler::null_check(Register, offset)
33d09e5: 8305247: On RISC-V generate_fixed_frame() sometimes generate a relativized locals value which is way too large
790aced: 8305100: [REDO] Clean up JavadocTokenizer
2e91585: 8303123: Add line break opportunity to single type parameters
094e03d: 8299718: JavaDoc: Buttons to copy specific documentation URL are not accessible
4de24cd: 8303210: [linux, Windows] Make UseSystemMemoryBarrier available as product flag
336a23e: 8303229: JFR: Preserve disk repository after exit
ecec611: 8283404: [macos] a11y : Screen magnifier does not show JMenu name
aa76210: 8304893: Link Time Optimization with gcc can be faster
b8c748d: 8294266: Add a way to pre-touch java thread stacks
... and 54 more: https://git.openjdk.org/jdk/compare/1683a63a7df6eb3bd71cd9d0a7ab7081b92107c4...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@LanceAndersen, @dfuch, @Martin-Buchholz) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

eirbjo · 2023-04-03T13:35:16Z

Updated the excessiveCENOffset comment to say just A ZipException is thrown when the 'End of Central Directory'(END) header has a CEN offset with an invalid value.

/integrate

openjdk · 2023-04-03T13:36:43Z

@eirbjo
Your change (at version ac4f2b4) is now ready to be sponsored by a Committer.

LanceAndersen · 2023-04-03T16:10:03Z

/sponsor

openjdk · 2023-04-03T16:11:49Z

Going to push as commit 85e3974.
Since your change was applied there have been 69 commits pushed to the master branch:

40aea04: 8278268: (ch) InputStream returned by Channels.newInputStream should have fast path for FileChannel targets
9b9b5a7: 8302323: Add repeat methods to StringBuilder/StringBuffer
dd7ca75: 8305478: [REDO] disable gtest/NMTGtests.java sub-tests failing due to JDK-8305414
f9827ad: 8288109: HttpExchangeImpl.setAttribute does not allow null value after JDK-8266897
6010de0: 8305417: disable gtest/NMTGtests.java sub-tests failing due to JDK-8305414
127afd3: 8241613: Suspicious calls to MacroAssembler::null_check(Register, offset)
33d09e5: 8305247: On RISC-V generate_fixed_frame() sometimes generate a relativized locals value which is way too large
790aced: 8305100: [REDO] Clean up JavadocTokenizer
2e91585: 8303123: Add line break opportunity to single type parameters
094e03d: 8299718: JavaDoc: Buttons to copy specific documentation URL are not accessible
... and 59 more: https://git.openjdk.org/jdk/compare/1683a63a7df6eb3bd71cd9d0a7ab7081b92107c4...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-04-03T16:12:05Z

@LanceAndersen @eirbjo Pushed as commit 85e3974.

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

Convert test CorruptedZipFiles to testNG and delete ZipFile constants…

bffeef4

… copies which are now obsolete.

eirbjo marked this pull request as ready for review February 14, 2023 17:47

openjdk bot added the core-libs core-libs-dev@openjdk.org label Feb 14, 2023

Trim whitespace and fix some spelling

f5609cb

LanceAndersen reviewed Feb 27, 2023

View reviewed changes

eirbjo added 3 commits February 27, 2023 22:53

Merge branch 'master' into corrupted-zip-files-ng

faf8def

Improve comments and method names to help future maintainers understa…

89d2f73

…nd what these tests verify.

Give the @BeforeMethod and @AfterMethod more meaningful names

e931869

Merge branch 'master' into corrupted-zip-files-ng

f2fd897

eirbjo marked this pull request as draft March 11, 2023 08:06

openjdk bot changed the title ~~Convert CorruptedZipFiles to testNG~~ 8304014: Convert test/jdk/java/util/zip/ZipFile/CorruptedZipFiles.java to testNG Mar 11, 2023

eirbjo added 2 commits March 29, 2023 11:06

Merge branch 'master' into corrupted-zip-files-ng

e6294bf

Use block comments instead of javadoc comments

26e184f

eirbjo marked this pull request as ready for review March 29, 2023 09:26

openjdk bot added the rfr Pull request is ready for review label Mar 29, 2023

Replace the u8, u16, u32 methods with using a little-endian ByteBuffe…

e46fc5b

…r. Collapse the checkZipException and checkZipExceptionInGetInputStream into one method by always consuming the InputStream. Add a block comment for the assertZipException method.

Junit version

892e166

eirbjo changed the title ~~8304014: Convert test/jdk/java/util/zip/ZipFile/CorruptedZipFiles.java to testNG~~ 8304014: Convert test/jdk/java/util/zip/ZipFile/CorruptedZipFiles.java to junit Mar 29, 2023

dfuch suggested changes Mar 29, 2023

View reviewed changes

eirbjo added 5 commits March 29, 2023 21:01

Update annotations to junit 5

01c25e8

Use ByteBuffer when manipulating multi-byte fields

f58b57a

Document the ByteBuffer with a comment

51e8883

Use the "Validate that.." comment style

f8c2cca

Document the structure of the 'good' ZIP

e32052f

LanceAndersen reviewed Mar 31, 2023

View reviewed changes

eirbjo added 2 commits March 31, 2023 20:03

this -> the

4847083

a A -> a

3dde45f

Martin-Buchholz reviewed Mar 31, 2023

View reviewed changes

eirbjo added 2 commits March 31, 2023 21:46

Spell immediately correctly

c366b65

Remove the "Validate that " comment prefix

3b6d857

LanceAndersen approved these changes Apr 2, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 2, 2023

Simplify the comment for excessiveCENOffset

ac4f2b4

openjdk bot added the sponsor Pull request is ready to be sponsored label Apr 3, 2023

openjdk bot added the integrated Pull request has been integrated label Apr 3, 2023

openjdk bot closed this Apr 3, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Apr 3, 2023

Conversation

eirbjo commented Feb 14, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Feb 14, 2023

Uh oh!

openjdk bot commented Feb 14, 2023

Uh oh!

LanceAndersen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eirbjo commented Feb 28, 2023

Uh oh!

eirbjo commented Mar 11, 2023

Uh oh!

openjdk bot commented Mar 11, 2023

Uh oh!

mlbridge bot commented Mar 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

dfuch commented Mar 29, 2023

Uh oh!

eirbjo commented Mar 29, 2023

Uh oh!

eirbjo commented Mar 29, 2023

Uh oh!

LanceAndersen commented Mar 29, 2023

Uh oh!

LanceAndersen commented Mar 29, 2023

Uh oh!

eirbjo commented Mar 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dfuch left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eirbjo commented Feb 14, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 29, 2023 •

edited

Loading

eirbjo commented Mar 29, 2023 •

edited

Loading

dfuch left a comment •

edited

Loading

eirbjo Mar 29, 2023 •

edited

Loading

LanceAndersen Mar 31, 2023 •

edited

Loading