Re: RFR: 8305945: (zipfs) Opening a directory to get input stream produces incorrect exception message [v5]
On Mon, 17 Apr 2023 17:04:32 GMT, Alan Bateman wrote: > What you have is fine, I'm just making the point that the exception message > is somewhat secondary I guess this is also a question of testing the interface vs. testing the implementation. To get good coverage of validation scenarios, I often find it quite useful to test on the actual message. This makes the test overspecific to the interface, but it helps make sure that each error condition is covered independently. Without asserting on the actual message, it can be hard to know if you are being shadowed by another higher level error condition. I've seen this a lot when adding test coverage for various ZIP processing code. - PR Review Comment: https://git.openjdk.org/jdk/pull/13482#discussion_r1169058586
Re: RFR: 8305945: (zipfs) Opening a directory to get input stream produces incorrect exception message [v5]
On Sun, 16 Apr 2023 10:27:07 GMT, Lance Andersen wrote: >> Okay, although I assume this test will fail if it throws a more general >> IOException (it's allowed to do that) or there is any adjustment to the >> error message. > > Yes, in the unlikely event that either FileSystemException or the code > below from zipfs is changed, then yes the error message could evolve > > > > if (e.isDir()) > throw new FileSystemException(getString(path), null, "is a > directory"); > > > I do not have a strong preference once way or the other and am happy to > remove the message validation if that is what you prefer. I kept the > validation as the original issue was surrounding the actual message being > generated which aligns with the text included by zipfs when it threw the > exception when obtaining the inputstream > > I think it is worth mentioning that we have other tests that also validate > exception messages(and we recently updated a couple zip tests because the > message changed).So if we are going to move in the direction of not > validating exception messages in tests, we should probably consider adding > verbiage to the developers guide to provide that guidance and be consistent > within the test suite > > Let me know your preference and thank you for your thoughts as it is a useful > discussion What you have is fine, I'm just making the point that the exception message is somewhat secondary, the important test is that if FileSystemException is thrown then its getOtherFile return should null as there is only one file in this operation. - PR Review Comment: https://git.openjdk.org/jdk/pull/13482#discussion_r1169034373
Re: RFR: 8305945: (zipfs) Opening a directory to get input stream produces incorrect exception message [v5]
On Sun, 16 Apr 2023 08:39:32 GMT, Alan Bateman wrote: >> added validation for other file being null but also kept the existing >> validation of the message > > Okay, although I assume this test will fail if it throws a more general > IOException (it's allowed to do that) or there is any adjustment to the error > message. Yes, in the unlikely event that either FileSystemException or the code below from zipfs is changed, then yes the error message could evolve if (e.isDir()) throw new FileSystemException(getString(path), null, "is a directory"); I do not have a strong preference once way or the other and am happy to remove the message validation if that is what you prefer. I kept the validation as the original issue was surrounding the actual message being generated which aligns with the text included by zipfs when it threw the exception when obtaining the inputstream I think it is worth mentioning that we have other tests that also validate exception messages(and we recently updated a couple zip tests because the message changed).So if we are going to move in the direction of not validating exception messages in tests, we should probably consider adding verbiage to the developers guide to provide that guidance and be consistent within the test suite Let me know your preference and thank you for your thoughts as it is a useful discussion - PR Review Comment: https://git.openjdk.org/jdk/pull/13482#discussion_r1167813939
Re: RFR: 8305945: (zipfs) Opening a directory to get input stream produces incorrect exception message [v5]
On Sat, 15 Apr 2023 10:24:58 GMT, Lance Andersen wrote:
>> test/jdk/jdk/nio/zipfs/ZipFSDirectoryExceptionMessageTest.java line 96:
>>
>>> 94: var file = zipfs.getPath(DIRECTORY_NAME);
>>> 95: var x = assertThrows(FileSystemException.class, () ->
>>> Files.newInputStream(file));
>>> 96: assertEquals(DIR_EXCEPTION_MESSAGE, x.getMessage());
>>
>> FYI, test doesn't need to depend the exception message. Checking
>> FileSystemException::getOtherFile returns null would be a more robust way of
>> checking that it didn't provide a second file by mistake, e.g.
>>
>>try {
>> Files.newInputStream(DIRECTORY_NAME);
>> fail();
>> } catch (FileSystemException e) {
>> assertNull(e.getOtherFile());
>> } catch (IOException ioe) {
>> // allowed
>> }
>
> added validation for other file being null but also kept the existing
> validation of the message
Okay, although I assume this test will fail if it throws a more general
IOException (it's allowed to do that) or there is any adjustment to the error
message.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13482#discussion_r1167784301
Re: RFR: 8305945: (zipfs) Opening a directory to get input stream produces incorrect exception message [v5]
On Sat, 15 Apr 2023 05:43:09 GMT, Alan Bateman wrote:
>> Lance Andersen has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> re-arrage assertEquals params
>
> test/jdk/jdk/nio/zipfs/ZipFSDirectoryExceptionMessageTest.java line 96:
>
>> 94: var file = zipfs.getPath(DIRECTORY_NAME);
>> 95: var x = assertThrows(FileSystemException.class, () ->
>> Files.newInputStream(file));
>> 96: assertEquals(DIR_EXCEPTION_MESSAGE, x.getMessage());
>
> FYI, test doesn't need to depend the exception message. Checking
> FileSystemException::getOtherFile returns null would be a more robust way of
> checking that it didn't provide a second file by mistake, e.g.
>
>try {
> Files.newInputStream(DIRECTORY_NAME);
> fail();
> } catch (FileSystemException e) {
> assertNull(e.getOtherFile());
> } catch (IOException ioe) {
> // allowed
> }
added validation for other file being null but also kept the existing
validation of the message
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13482#discussion_r1167471233
Re: RFR: 8305945: (zipfs) Opening a directory to get input stream produces incorrect exception message [v6]
> Please review this trivial change when ZipFS returns the wrong > java.nio.file.FileSystemException message due the the parameters being > reversed. > > I also included a simple junit test as part of the fix. > > Mach5 tiers1-3 are clean > > Best > Lance Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: added valiation that other file is null - Changes: - all: https://git.openjdk.org/jdk/pull/13482/files - new: https://git.openjdk.org/jdk/pull/13482/files/3067fd17..74534d59 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13482=05 - incr: https://webrevs.openjdk.org/?repo=jdk=13482=04-05 Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/13482.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13482/head:pull/13482 PR: https://git.openjdk.org/jdk/pull/13482
Re: RFR: 8305945: (zipfs) Opening a directory to get input stream produces incorrect exception message [v5]
On Sat, 15 Apr 2023 00:36:35 GMT, Lance Andersen wrote:
>> Please review this trivial change when ZipFS returns the wrong
>> java.nio.file.FileSystemException message due the the parameters being
>> reversed.
>>
>> I also included a simple junit test as part of the fix.
>>
>> Mach5 tiers1-3 are clean
>>
>> Best
>> Lance
>
> Lance Andersen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> re-arrage assertEquals params
test/jdk/jdk/nio/zipfs/ZipFSDirectoryExceptionMessageTest.java line 96:
> 94: var file = zipfs.getPath(DIRECTORY_NAME);
> 95: var x = assertThrows(FileSystemException.class, () ->
> Files.newInputStream(file));
> 96: assertEquals(DIR_EXCEPTION_MESSAGE, x.getMessage());
FYI, test doesn't need to depend the exception message. Checking
FileSystemException::getOtherFile returns null would be a more robust way of
checking that it didn't provide a second file by mistake, e.g.
try {
Files.newInputStream(DIRECTORY_NAME);
fail();
} catch (FileSystemException e) {
assertNull(e.getOtherFile());
} catch (IOException ioe) {
// allowed
}
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13482#discussion_r1167384199
Re: RFR: 8305945: (zipfs) Opening a directory to get input stream produces incorrect exception message [v5]
> Please review this trivial change when ZipFS returns the wrong > java.nio.file.FileSystemException message due the the parameters being > reversed. > > I also included a simple junit test as part of the fix. > > Mach5 tiers1-3 are clean > > Best > Lance Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: re-arrage assertEquals params - Changes: - all: https://git.openjdk.org/jdk/pull/13482/files - new: https://git.openjdk.org/jdk/pull/13482/files/f91f8f66..3067fd17 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13482=04 - incr: https://webrevs.openjdk.org/?repo=jdk=13482=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13482.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13482/head:pull/13482 PR: https://git.openjdk.org/jdk/pull/13482
Re: RFR: 8305945: (zipfs) Opening a directory to get input stream produces incorrect exception message [v4]
On Fri, 14 Apr 2023 21:59:33 GMT, Lance Andersen wrote: >> Please review this trivial change when ZipFS returns the wrong >> java.nio.file.FileSystemException message due the the parameters being >> reversed. >> >> I also included a simple junit test as part of the fix. >> >> Mach5 tiers1-3 are clean >> >> Best >> Lance > > Lance Andersen has updated the pull request incrementally with one additional > commit since the last revision: > > Address additional feedback test/jdk/jdk/nio/zipfs/ZipFSDirectoryExceptionMessageTest.java line 96: > 94: var file = zipfs.getPath(DIRECTORY_NAME); > 95: var x = assertThrows(FileSystemException.class, () -> > Files.newInputStream(file)); > 96: assertEquals(x.getMessage(), DIR_EXCEPTION_MESSAGE); JUnit asserts have the expected parameter first followed by the actual. - PR Review Comment: https://git.openjdk.org/jdk/pull/13482#discussion_r1167305606
Re: RFR: 8305945: (zipfs) Opening a directory to get input stream produces incorrect exception message [v4]
On Fri, 14 Apr 2023 21:30:40 GMT, Eirik Bjorsnos wrote: >> Lance Andersen has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Address additional feedback > > test/jdk/jdk/nio/zipfs/ZipFSDirectoryExceptionMessageTest.java line 40: > >> 38: * @test >> 39: * @bug 8305945 >> 40: * @summary Validate that Zip FS provides the correct exception message > > This summary could perhaps be a bit more specific about which condition / > message it is testing. done > test/jdk/jdk/nio/zipfs/ZipFSDirectoryExceptionMessageTest.java line 56: > >> 54: * Zip file to create >> 55: */ >> 56: public static final String ZIP_FILE = "directoryExceptionTest.zip"; > > Maybe ZIP_FILE could be a Path, to avoid Path.of(ZIP_FILE) wrapping later in > the test? I often do that but chose not to when I created the test but have updated per your suggestion - PR Review Comment: https://git.openjdk.org/jdk/pull/13482#discussion_r1167297960 PR Review Comment: https://git.openjdk.org/jdk/pull/13482#discussion_r1167299017
Re: RFR: 8305945: (zipfs) Opening a directory to get input stream produces incorrect exception message [v4]
> Please review this trivial change when ZipFS returns the wrong > java.nio.file.FileSystemException message due the the parameters being > reversed. > > I also included a simple junit test as part of the fix. > > Mach5 tiers1-3 are clean > > Best > Lance Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: Address additional feedback - Changes: - all: https://git.openjdk.org/jdk/pull/13482/files - new: https://git.openjdk.org/jdk/pull/13482/files/8fbf69aa..f91f8f66 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13482=03 - incr: https://webrevs.openjdk.org/?repo=jdk=13482=02-03 Stats: 6 lines in 1 file changed: 1 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/13482.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13482/head:pull/13482 PR: https://git.openjdk.org/jdk/pull/13482
Re: RFR: 8305945: (zipfs) Opening a directory to get input stream produces incorrect exception message [v3]
On Fri, 14 Apr 2023 21:40:17 GMT, Lance Andersen wrote: >> Please review this trivial change when ZipFS returns the wrong >> java.nio.file.FileSystemException message due the the parameters being >> reversed. >> >> I also included a simple junit test as part of the fix. >> >> Mach5 tiers1-3 are clean >> >> Best >> Lance > > Lance Andersen has updated the pull request incrementally with one additional > commit since the last revision: > > Minor wordsmithing of test comment LGTM. Leaving some minor comments on the test. test/jdk/jdk/nio/zipfs/ZipFSDirectoryExceptionMessageTest.java line 40: > 38: * @test > 39: * @bug 8305945 > 40: * @summary Validate that Zip FS provides the correct exception message This summary could perhaps be a bit more specific about which condition / message it is testing. test/jdk/jdk/nio/zipfs/ZipFSDirectoryExceptionMessageTest.java line 56: > 54: * Zip file to create > 55: */ > 56: public static final String ZIP_FILE = "directoryExceptionTest.zip"; Maybe ZIP_FILE could be a Path, to avoid Path.of(ZIP_FILE) wrapping later in the test? - Marked as reviewed by eir...@github.com (no known OpenJDK username). PR Review: https://git.openjdk.org/jdk/pull/13482#pullrequestreview-1386198889 PR Review Comment: https://git.openjdk.org/jdk/pull/13482#discussion_r1167287569 PR Review Comment: https://git.openjdk.org/jdk/pull/13482#discussion_r1167286862
Re: RFR: 8305945: (zipfs) Opening a directory to get input stream produces incorrect exception message [v3]
> Please review this trivial change when ZipFS returns the wrong > java.nio.file.FileSystemException message due the the parameters being > reversed. > > I also included a simple junit test as part of the fix. > > Mach5 tiers1-3 are clean > > Best > Lance Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: Minor wordsmithing of test comment - Changes: - all: https://git.openjdk.org/jdk/pull/13482/files - new: https://git.openjdk.org/jdk/pull/13482/files/b1dfab8b..8fbf69aa Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13482=02 - incr: https://webrevs.openjdk.org/?repo=jdk=13482=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13482.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13482/head:pull/13482 PR: https://git.openjdk.org/jdk/pull/13482
Re: RFR: 8305945: (zipfs) Opening a directory to get input stream produces incorrect exception message [v2]
On Fri, 14 Apr 2023 21:07:37 GMT, Naoto Sato wrote: > Looks good, Lance. Nit: copyright year -> 2023 Geez, working in too many workspaces. Thank you, just pushed the update - PR Comment: https://git.openjdk.org/jdk/pull/13482#issuecomment-1509300245
Re: RFR: 8305945: (zipfs) Opening a directory to get input stream produces incorrect exception message [v2]
On Fri, 14 Apr 2023 21:08:28 GMT, Christian Stein wrote: > Looks good to me. Thank you Christian - PR Comment: https://git.openjdk.org/jdk/pull/13482#issuecomment-1509301103
Re: RFR: 8305945: (zipfs) Opening a directory to get input stream produces incorrect exception message [v2]
> Please review this trivial change when ZipFS returns the wrong > java.nio.file.FileSystemException message due the the parameters being > reversed. > > I also included a simple junit test as part of the fix. > > Mach5 tiers1-3 are clean > > Best > Lance Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: Update the copyright year - Changes: - all: https://git.openjdk.org/jdk/pull/13482/files - new: https://git.openjdk.org/jdk/pull/13482/files/e99a91bf..b1dfab8b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13482=01 - incr: https://webrevs.openjdk.org/?repo=jdk=13482=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13482.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13482/head:pull/13482 PR: https://git.openjdk.org/jdk/pull/13482
Re: RFR: 8305945: (zipfs) Opening a directory to get input stream produces incorrect exception message
On Fri, 14 Apr 2023 20:24:00 GMT, Lance Andersen wrote: > Please review this trivial change when ZipFS returns the wrong > java.nio.file.FileSystemException message due the the parameters being > reversed. > > I also included a simple junit test as part of the fix. > > Mach5 tiers1-3 are clean > > Best > Lance Looks good to me. - Marked as reviewed by cstein (Committer). PR Review: https://git.openjdk.org/jdk/pull/13482#pullrequestreview-1386182752
Re: RFR: 8305945: (zipfs) Opening a directory to get input stream produces incorrect exception message
On Fri, 14 Apr 2023 20:24:00 GMT, Lance Andersen wrote: > Please review this trivial change when ZipFS returns the wrong > java.nio.file.FileSystemException message due the the parameters being > reversed. > > I also included a simple junit test as part of the fix. > > Mach5 tiers1-3 are clean > > Best > Lance Looks good, Lance. Nit: copyright year -> 2023 - Marked as reviewed by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13482#pullrequestreview-1386182062
