On Wed, 25 Feb 2026 06:50:46 GMT, SendaoYan <[email protected]> wrote:
>> Daniel Fuchs has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains three additional
>> commits since the last revision:
>>
>> - Review feedback
>> - Merge branch 'master' into junit-httpclient-misc-8378595
>> - 8378595: Refactor miscellaneous tests under test/jdk/java/net/httpclient
>> from TestNG to JUnit
>
> test/jdk/java/net/httpclient/PathSubscriber/BodyHandlerOfFileDownloadTest.java
> line 116:
>
>> 114: { http3URI, defaultFsPath, MSG, true },
>> 115: { http3URI, defaultFsPath, MSG, false },
>> 116:
>
> Could we remove this empty line
Done.
> test/jdk/java/net/httpclient/security/filePerms/FileProcessorPermissionTest.java
> line 38:
>
>> 36:
>> 37: import org.junit.jupiter.api.Test;
>> 38: import static org.junit.jupiter.api.Assertions.*;
>
> This import "import static org.junit.jupiter.api.Assertions.*;" seems useless.
Done
> test/jdk/java/net/httpclient/security/filePerms/SecurityBeforeFile.java line
> 44:
>
>> 42: import org.junit.jupiter.params.ParameterizedTest;
>> 43: import org.junit.jupiter.params.provider.MethodSource;
>> 44: import static org.junit.jupiter.api.Assertions.*;
>
> In this file, seems that we can import
> "org.junit.jupiter.api.Assertions.fail" explicitly rather than import all the
> assertion method.
Done.
> test/jdk/java/net/httpclient/security/filePerms/SecurityBeforeFile.java line
> 79:
>
>> 77: try {
>> 78: BodyHandlers.ofFileDownload(p, openOptions);
>> 79: fail("UNEXPECTED, file " + p.toString() + " exists?");
>
> Seems that use assertThrows better than try-fail-catch
>
>
> assertThrows(FileNotFoundException.class, () -> {
> BodyPublishers.ofFile(p);
> });
I'd rather keep things the way they are as they provide additional logging and
diagnosis compare to plain usage of `assertThrows`.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/29900#discussion_r2852610082
PR Review Comment: https://git.openjdk.org/jdk/pull/29900#discussion_r2852609527
PR Review Comment: https://git.openjdk.org/jdk/pull/29900#discussion_r2852608937
PR Review Comment: https://git.openjdk.org/jdk/pull/29900#discussion_r2852608365
