On Tue, 4 Feb 2025 07:24:20 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> Volkan Yazici 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: >> >> - Merge remote-tracking branch 'upstream/master' into >> HttpRequest-Builder-copy-Test >> - Update copyright year >> - Verify `copy()` in `HttpRequestBuilderTest` > > test/jdk/java/net/httpclient/HttpRequestBuilderTest.java line 407: > >> 405: >> 406: // Verify copied _references_ >> 407: assert request.uri().equals(copiedRequest.uri()); > > Hello Volkan, within the JDK build, we launch these jtreg tests with > assertions enabled by default. However, from what I see in the build files, > that's configurable. So it may not always be the case that assertions are > enabled for these tests. > > Furthermore, I don't remember seeing `assert` being used within the test code > to verify the test expectations. So I think it would be better to do a > conditional check here and explicitly throw a `AssertionError` if that check > fails, like we do in some other places of this test. Fixed in 5eddbf7d4477cf997111b4dae8b3cb112216fadc. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/23415#discussion_r1940959574
