On Mon, 30 Oct 2023 14:10:25 GMT, Lance Andersen <lan...@openjdk.org> wrote:
>> Eirik Bjorsnos has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains 14 additional >> commits since the last revision: >> >> - Run CenSizeTooLarge automatically, since it no longer requires excessive >> memory or disk space >> - Use a SparseOutputStream to reduce required diskspace from 2GB to 4K. >> SparseOutputStream writes 'holes' instead of actual bytes until the final >> CEN is written. >> - TestTooManyEntries is renamed to CenSizeTooLarge >> - Merge branch 'master' into cen-size-too-large >> - MAX_EXTRA_FIELD_SIZE can be better expressed as 0xFFFF >> - Bring back '@requires sun.arch.data.model == 64' for now >> - Spell 'specified' correctly >> - Give test method a long, meaningful name >> - Remove blank line >> - Make CEN headers maximally large such that we need fewer of them and save >> memory >> - ... and 4 more: https://git.openjdk.org/jdk/compare/f5cb971b...9629b8d2 > > test/jdk/java/util/zip/ZipFile/CenSizeTooLarge.java line 33: > >> 31: import org.testng.annotations.BeforeTest; >> 32: import org.testng.annotations.Test; >> 33: > > As this will be a new test, could you please consider converting to junit. - Added a comment explaining the chosen value for the constant, referencing APPNOTE.TXT - Converted the test to JUnit (even remembered to swap argument order of assertEquals!) > test/jdk/java/util/zip/ZipFile/CenSizeTooLarge.java line 139: > >> 137: public void centralDirectoryTooLargeToFitInByteArray() { >> 138: ZipException ex = expectThrows(ZipException.class, () -> new >> ZipFile(hugeZipFile)); >> 139: assertEquals(ex.getMessage(), "invalid END header (central >> directory size too large)"); > > Could be have the expected message a class constant so that if this message > ever changes it is easier to find/update Extracted the expected ZipException message to a class constant. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/12991#discussion_r1376407892 PR Review Comment: https://git.openjdk.org/jdk/pull/12991#discussion_r1376408869
