On Sun, 31 Dec 2023 18:07:33 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. Thanks for taking this on. With the conversion, I probably would would at separating out the test runs for ZipFile and ZipInputStream and introduce a DataProvider. This will just make it a bit clearer to identify an error that might be specific to ZipFile and/or ZipInputStream.. ------------- PR Review: https://git.openjdk.org/jdk/pull/17207#pullrequestreview-1799692059
