RFR: 8298588: WebSockets: HandshakeUrlEncodingTest unnecessarily depends on a response body [v2]

[Michal Karm Babacek]

On Wed, 14 Dec 2022 11:30:27 GMT, Daniel Fuchs <dfuchs at openjdk.org> wrote:

>> Michal Karm Babacek has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Makes test independent on JDK-8240666
>
> test/jdk/java/net/httpclient/websocket/HandshakeUrlEncodingTest.java line 127:
> 
>> 125:                 assertEquals(rawQuery, expectedRawQuery);
>> 126:                 if (wse.getResponse().body() != null &&
>> 127:                         (wse.getResponse().body().getClass().equals(String.class))) {
> 
> In the case of the mainline there MUST be a body - otherwise that is a bug in the implementation.
> I believe the mainline test should therefore asserts that the body is present and equals to what it expects.
> 
> There is no requirement that a test in the mainline be the same than in previous releases, especially when the mainline test makes use of a functionality that is not present in those releases.
> 
> It should be perfectly fine to have the mainline test checks two things, and the update release test only one thing, because the second thing is not implemented in the update release.
> 
> If you modify the mainline test to simply add a check to assert that the response URI is what the text expects, then the diff between the update release test (that only check the response URI) and the mainline test after your fix (that checks both the response URI and the body) should naturally show that the mainline test has one additional test compared to the update release test - which is making use of a functionality only available in the mainline. 
> 
> I would even suggest to modify the handler in the update release to not write anything into the response body since the body is discarded by the client implementation in that release.
> 
> The reason I'm a bit insistent on keeping the check on the body string is that it's the only thing that can reliably assert that the server has received the correct URI. Conceptually, the URI in the HttpResponse could have be composed by the client implementation itself. Writing the URI in the response body was a convenient way to check what the server had actually received.

Thx @dfuch for the comment.

> In the case of the mainline there MUST be a body - otherwise that is a bug in the implementation. I believe the mainline test should therefore asserts that the body is present and equals to what it expects.

I can see your point and I changed the test in my latest commit. The reason for my original approach for omitting it and for the subsequent step of checking for `body` presence was that the behavior of not/dropping body has its own test in WSHandshakeExceptionTest.java.

> 
> There is no requirement that a test in the mainline be the same than in previous releases, especially when the mainline test makes use of a functionality that is not present in those releases.
> 

I see. I might got confused by comments on the [jdk11u-dev](https://github.com/openjdk/jdk11u-dev/pull/1558#pullrequestreview-1201689329) thread:

The thing to do is use the buggy code from the original commit in this backport, 
fix the problem in tip, then backport that fix to 19u, 17u, and 11u.


> It should be perfectly fine to have the mainline test checks two things, and the update release test only one thing, because the second thing is not implemented in the update release.
> 
> If you modify the mainline test to simply add a check to assert that the response URI is what the text expects, then the diff between the update release test (that only check the response URI) and the mainline test after your fix (that checks both the response URI and the body) should naturally show that the mainline test has one additional test compared to the update release test - which is making use of a functionality only available in the mainline.
> 
> I would even suggest to modify the handler in the update release to not write anything into the response body since the body is discarded by the client implementation in that release.
> 

The diff between the test in this PR (tip, mailine) and the one in JDK 11 one [pull/1558](https://github.com/openjdk/jdk11u-dev/pull/1558) is https://editor.mergely.com/dOyW1m6w/ .

> The reason I'm a bit insistent on keeping the check on the body string is that it's the only thing that can reliably assert that the server has received the correct URI. Conceptually, the URI in the HttpResponse could have be composed by the client implementation itself. Writing the URI in the response body was a convenient way to check what the server had actually received.

I am glad you are providing feedback.

-------------

PR: https://git.openjdk.org/jdk/pull/11486