On Tue, 2 Jan 2024 09:31:16 GMT, Eirik Bjørsnøs <eir...@openjdk.org> wrote:
>> Please review this test-only PR which adds test coverage for >> `ZipFile.getEntry` under certain charset conditions. >> >> When `ZipFile.getEntry` is called for an entry which has the `Language >> encoding flag` general purpose bit flag set, then `ZipCoder.UTF8` is used >> unconditionally, even when a different charset was supplied to the `ZipFile` >> constructor. >> >> It turns out we do not have any testing for this particular case, as can be >> verified by commenting out the following line of code in >> `ZipFile.Source.getEntryPos`: >> >> >> //ZipCoder zc = zipCoderForPos(pos); >> ``` >> >> and then running `make test TEST="test/jdk/java/util/zip"` >> >> The current test verifies that the correct ZipCoder is used by >> `ZipFile.entries()`, but does not exercise `ZipFile.getEntry` the same way. >> >> Seeing that [JDK-7009069](https://bugs.openjdk.org/browse/JDK-7009069) was >> (accidentally?) fixed by >> [JDK-8243469](https://bugs.openjdk.org/browse/JDK-8243469), I think it is >> worthwhile to add explicit testing for this case to avoid regressions. >> >> While visiting `ZipCoding.java`, I took the opportunity to convert it to >> JUnit 5. The conversion and modernization of the code is done in the first >> commit 1384850ed51ec845af06dd6d13616f20f8bbaa6a in this PR, while the second >> commit 1776b258b0fe8383709ae0c095f2631a4e6237f6 actually adds the code >> required to verify the `Language encoding flag` condition for >> `ZipFile.getEntry`. >> >> Testing: Verified that the test indeed fails when >> `ZipFile.Source.getEntryPos` is updated to use the ZipFile's ZipCoder as >> suggested above. > > Eirik Bjørsnøs has updated the pull request incrementally with one additional > commit since the last revision: > > Add more cases for 'language encoding' bit set, opened with a different > encoding Looks good over all. One minor comment to consider before pushing test/jdk/java/util/zip/ZipCoding.java line 130: > 128: assertEquals(name, e.getName()); > 129: assertNull(e.getComment()); // No comment in the LOC header > 130: assertArrayEquals(ENTRY_DATA, zis.readAllBytes(), "ZipIS > content doesn't match!"); Minor Nit, but perhaps changes the "ZipIS content..." message to make it clearer as it is not what is clear what ZipIS is... ------------- Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17207#pullrequestreview-1800709897 PR Review Comment: https://git.openjdk.org/jdk/pull/17207#discussion_r1439626614
