On Wed, 30 Nov 2022 22:15:14 GMT, Lance Andersen <lan...@openjdk.org> wrote:
>> Liam Miller-Cushon has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Use TestNG, and assert on the compression method
>
> test/jdk/tools/jar/ManifestDirectoryCompression.java line 52:
>
>> 50:
>> 51: /** Remove dirs & files needed for test. */
>> 52: private static void cleanup(Path dir) {
>
> This can leverage the @afterMethod annotation if the code is reworked
> slightly to make it more TestNG friendly
Thanks, done
> test/jdk/tools/jar/ManifestDirectoryCompression.java line 60:
>
>> 58: }
>> 59: Files.delete(dir);
>> 60: } catch (IOException e) {
>
> Have the method throw IOException and you do not need the catch block
Unfortunately it's recursing on `cleanup` in the lambda, so it can't throw
checked exceptions without more refactoring. This is imitating the recursive
deletion approach in another jar test, I'm happy to swap this out if you'd
prefer a different approach
> test/jdk/tools/jar/ManifestDirectoryCompression.java line 72:
>
>> 70: doTest(topDir.resolve("test.jar"), entry);
>> 71: } finally {
>> 72: cleanup(topDir);
>
> See comment above regarding cleanup()
Thanks, I moved the cleanup into a separate `@AfterMethod`
> test/jdk/tools/jar/ManifestDirectoryCompression.java line 83:
>
>> 81: try (JarFile jarFile = new JarFile(jar.toFile())) {
>> 82: ZipEntry zipEntry = jarFile.getEntry("META-INF/");
>> 83: assertEquals(zipEntry.getMethod(), ZipEntry.STORED);
>
> Probably worth verifying zipEntry is not null so you do not get an NPE
Done
-------------
PR: https://git.openjdk.org/jdk/pull/11441
