On Mon, 2 Feb 2026 20:21:44 GMT, Volkan Yazici <[email protected]> wrote:
>> test/jdk/com/sun/net/httpserver/ContextPathMatcherPathPrefixTest.java line
>> 52:
>>
>>> 50:
>>> 51: /*
>>> 52: * @test id=withProperty
>>
>> Suggestion:
>>
>> * @test id=withPathPrefix
>
> I think `withPathPrefix` is misleading, since `id=default` test is also using
> path-prefix matching. Note that this file only tests path-prefix matching
> using all possible configuration combinations:
>
> 1. Using defaults (`id=default`)
> 2. Providing a property (`id=withProperty`)
> 3. Providing an invalid value in the property, and hence, observing the
> fallback to the default (`id=withInvalidProperty`)
OK. The typical way of handling this is to have a single test that tests all
possible values of the property, and that's why the name looked misleading to
me at first sight. But since you split the testing into different tests I guess
that's OK.
>> test/jdk/com/sun/net/httpserver/ContextPathMatcherPathPrefixTest.java line
>> 72:
>>
>>> 70: * ${test.main.class}
>>> 71: */
>>> 72:
>>
>> Could we simply have another test comment to test with
>> `-Dsun.net.httpserver.pathMatcher=stringPrefix` instead of having a separate
>> test class?
>
> I don't think we can have that _simply_. To test string-prefix matching in
> the same file with path-prefix matching, I need to add a _"If we're expecting
> path-prefix matching, do this; otherwise, do this"_ branching to every single
> `@Test` method, and this was the very reason I split the tests into two files
> and reused the boilerplate (e.g., the `Infra` class) between the two.
> Nevertheless, to further simplify `ContextPathMatcherStringPrefixTest`, I
> make it extend from `ContextPathMatcherPathPrefixTest` in e55e6050466. If you
> have another suggestion, I'm all ears.
OK
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/29264#discussion_r2764610774
PR Review Comment: https://git.openjdk.org/jdk/pull/29264#discussion_r2764612223
