On Tue, 19 Dec 2023 21:34:02 GMT, Weibing Xiao <d...@openjdk.org> wrote:
>> Better Error Handling for Jar Tool Processing of "@File" <br> >> >> This is a new PR for this PR since the original developer left the team. See >> all of the review history at https://github.com/openjdk/jdk/pull/16423. >> >> Thank you. > > Weibing Xiao has updated the pull request incrementally with one additional > commit since the last revision: > > updat testing cases Thank you Weibing for these updates. I think this is now almost there. One final request, would you be able to add a test for the `-u` option as well? test/jdk/tools/jar/InputFilesTest.java line 161: > 159: * IOException is triggered as expected. > 160: */ > 161: @Test(expectedExceptions = {IOException.class}) Hello Weibing, I think using `expectedExceptions` here at the method level is too wide for this test, so might be better to remove it. I see that you use a catch block (at the right place) in this test method to verify you have received the correct `IOException`. What you could do is, don't rethrow it if it matches the expected IOException: try { jar("cf test.jar existingTestFile.txt nonExistentTestFile.txt"); } catch (IOException e) { // expected to have received an IOException due to tool // returning non-zero exit code Assert.assertEquals(e.getMessage().trim(), "nonExistentTestFile.txt : no such file or directory"); Assert.assertTrue(Files.notExists(Path.of("test.jar")), "Jar file should not be created."); } Same comment for the other new test method. test/jdk/tools/jar/InputFilesTest.java line 171: > 169: Assert.assertTrue(Files.notExists(Path.of("test.jar")), "Jar > file should not be created."); > 170: throw e; > 171: } There should be a `fail("jar tool unexpectedly completed successfully")` here so that we assert that the `jar` call actually fails. Same comment for the other new test method. ------------- PR Comment: https://git.openjdk.org/jdk/pull/17088#issuecomment-1863693788 PR Review Comment: https://git.openjdk.org/jdk/pull/17088#discussion_r1432082929 PR Review Comment: https://git.openjdk.org/jdk/pull/17088#discussion_r1432083748
