RFR: 8318971 : Better Error Handling for Jar Tool When Processing Non-existent Files [v3]

[Jaikiran Pai]

On Tue, 19 Dec 2023 21:34:02 GMT, Weibing Xiao <duke at 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