On Fri, 10 Apr 2026 11:32:53 GMT, Marcono1234 <[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 four additional
>> commits since the last revision:
>>
>> - Merge branch 'master' into junit-url-http-8381767
>> - Merge branch 'master' into junit-url-http-8381767
>> - Merge branch 'master' into junit-url-http-8381767
>> - 8381767: Refactor various java/net/*[URL/Http]*/ TestNG tests to use JUnit
>
> test/jdk/java/net/HttpCookie/whitebox/java.base/java/net/MaxAgeExpires.java
> line 116:
>
>> 114: System.out.println("getMaxAge returned " +
>> cookie.getMaxAge());
>> 115: System.out.println("expectedAge was " + expectedAge);
>> 116: throw new RuntimeException("Test failed: wrong age");
>
> Should this be simplified?
>
>
> if (expectedAge != -1) {
> assertEquals(expectedAge, cookie.getMaxAge());
> }
Thanks. I'd prefer to keep the logging as it is. I might move it to System.err
though.
> test/jdk/java/net/HttpCookie/whitebox/java.base/java/net/MaxAgeExpires.java
> line 128:
>
>> 126: System.out.println("getMaxAge() returned " +
>> cookie.getMaxAge());
>> 127: throw new RuntimeException("Test failed: wrong hasExpired");
>> 128: }
>
> Should this be simplified?
>
> assertEquals(hasExpired, expired, "for getMaxAge() " + cookie.getMaxAge());
Same here.
> test/jdk/java/net/HttpURLConnection/HttpURLConnectionHeadersOrder.java line
> 56:
>
>> 54: public class HttpURLConnectionHeadersOrder {
>> 55: private static final String LOCAL_TEST_ENDPOINT = "/headertest";
>> 56: private static final String ERROR_MESSAGE_TEMPLATE = "Expected
>> Request Properties = %s, Actual Request Properties = %s";
>
> Is this custom error message redundant? JUnit's built-in assertion message
> will be nearly the same.
Good point.
> test/jdk/java/net/URLClassLoader/definePackage/SplitPackage.java line 93:
>
>> 91: } catch (IllegalArgumentException e) {
>> 92:
>> 93: }
>
> If possible replace this with `assertThrows` (in case that is the expected
> behavior?)?
Good catch.
> test/jdk/java/net/URLConnection/RequestProperties.java line 65:
>
>> 63: }
>> 64: return data;
>> 65: }
>
> Could possibly be simplified a lot by directly returning `List<String> urls`.
>
> See also [`MethodSource`
> doc](https://docs.junit.org/6.0.3/api/org.junit.jupiter.params/org/junit/jupiter/params/provider/MethodSource.html):
>> Each set of "arguments" within the "stream" can be supplied as an instance
>> of [...], or a single value if the parameterized test method accepts a
>> single argument.
Good point.
> test/jdk/java/net/URLConnection/SetDefaultUseCaches.java line 70:
>
>> 68:
>> 69: // set default for http to false and check
>> 70: URLConnection.setDefaultUseCaches("HTTP", false);
>
> Should there be an `@AfterAll` (or a `try-finally`) which reverts all these
> `URLConnection#setDefaultUseCaches` calls?
>
> Or is this not an issue because this runs in a separate JVM?
This is not an issue as the test runs with `/othervm` (precisely because it
modifies the global state of the VM).
> test/jdk/java/net/URLConnection/URLConnectionHeadersOrder.java line 68:
>
>> 66: String errorMessageTemplate = "Expected Request Properties = %s,
>> Actual Request Properties = %s";
>> 67: assertEquals(expectedRequestProps, actualRequestProps,
>> 68: String.format(errorMessageTemplate,
>> expectedRequestProps, actualRequestProps));
>
> Is this custom error message redundant because JUnit's one will be nearly
> identical?
Good point
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/30626#discussion_r3065383206
PR Review Comment: https://git.openjdk.org/jdk/pull/30626#discussion_r3065384102
PR Review Comment: https://git.openjdk.org/jdk/pull/30626#discussion_r3065385673
PR Review Comment: https://git.openjdk.org/jdk/pull/30626#discussion_r3065463660
PR Review Comment: https://git.openjdk.org/jdk/pull/30626#discussion_r3065489769
PR Review Comment: https://git.openjdk.org/jdk/pull/30626#discussion_r3065484693
PR Review Comment: https://git.openjdk.org/jdk/pull/30626#discussion_r3065487002
