On Wed, 8 Apr 2026 16:23:15 GMT, Daniel Fuchs <[email protected]> wrote:
>> Hi, may I get a review for this change that convert some more java/net tests
>> from TestNG to JUnit.
>>
>> In order to avoid a bigger changeset that would be harder to review, this
>> batch simply changes all TestNG tests under java/net that have either URL or
>> Http in their names.
>
> 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
Hopefully these comments are useful, if not please let me know. They are only
suggestions; feel free to ignore them, I am not an OpenJDK member.
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());
}
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());
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.
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?)?
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.
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?
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?
-------------
PR Review: https://git.openjdk.org/jdk/pull/30626#pullrequestreview-4089284746
PR Review Comment: https://git.openjdk.org/jdk/pull/30626#discussion_r3063936875
PR Review Comment: https://git.openjdk.org/jdk/pull/30626#discussion_r3063944386
PR Review Comment: https://git.openjdk.org/jdk/pull/30626#discussion_r3064014684
PR Review Comment: https://git.openjdk.org/jdk/pull/30626#discussion_r3064114285
PR Review Comment: https://git.openjdk.org/jdk/pull/30626#discussion_r3064144981
PR Review Comment: https://git.openjdk.org/jdk/pull/30626#discussion_r3064176789
PR Review Comment: https://git.openjdk.org/jdk/pull/30626#discussion_r3064186926
