RFR[8217825]: Verify @AfterTest is used correctly in WebSocket tests

Pavel Rappo pavel.rappo at oracle.com
Fri Sep 20 12:21:52 UTC 2019


Patrick,

Thank you for taking care of that!
Before you push, please add missing spaces to

    WebSocketTest.java:612
    WebSocketTest.java:713

(no need to update the webrev)

Nits:

WebSocketTest.java:808 does not abort WebSocket. Now, I understand that in normal circumstances we never expect a WebSocket to be created. However, for uniformity's sake, I would use abort here as well.

I understand that "single line" method formatting used in AutomaticPong.java:140+ might not be everyone's cup of tea, but it might add to readability in the case of a lengthy definition like that.

-Pavel

> On 20 Sep 2019, at 11:36, Daniel Fuchs <daniel.fuchs at oracle.com> wrote:
> 
> Hi Patrick,
> 
> Looks good to me!
> 
> best regards,
> 
> -- daniel
> 
> On 20/09/2019 11:27, Patrick Concannon wrote:
>> Hi Daniel,
>> Thanks for the feedback. That's a good idea. I've made those changes and I've included them in the webrev below.
>> http://cr.openjdk.java.net/~pconcannon/8217825/webrevs/webrev.01/
>> Kind regards,
>> Patrick
>> On 13/09/2019 15:47, Daniel Fuchs wrote:
>>> Hi Patrick,
>>> 
>>> I wonder if you should be using `try { } finally { }` to ensure
>>> that the websocket is closed too:
>>> 
>>>    var websocket = .....;
>>>    try {
>>>     // send some messages etc...
>>>    } finally {
>>>     websocket.abort();
>>>    }
>>> 
>>> best regards,
>>> 
>>> -- daniel
>>> 
>>> On 13/09/2019 15:36, Patrick Concannon wrote:
>>>> Hi, 
> 
> 
>>>> 
>>>> 
>>>> Would it be possible to have my fix for JDK-8217825 - Verify @AfterTest is used correctly in WebSocket tests - reviewed?
>>>> 
>>>> Following on from the discussion had here: https://mail.openjdk.java.net/pipermail/net-dev/2019-January/012141.html, I’ve refactored several websocket tests to explicitly close resources opened during each test method i.e. test servers and websockets.
>>>> 
>>>> Tests refactored:
>>>> - test/jdk/java/net/httpclient/websocket/AutomaticPong.java
>>>> - test/jdk/java/net/httpclient/websocket/WebsocketTest.java
>>>> - test/jdk/java/net/httpclient/websocket/SendTest.java
>>>> - test/jdk/java/net/httpclient/websocket/Abort.java
>>>> 
>>>> 
>>>> Further information on this bug can be found here: https://bugs.openjdk.java.net/browse/JDK-8217825
>>>> 
> 
> Webrev for fix: 
>>>> http://cr.openjdk.java.net/~pconcannon/8217825/webrevs/webrev.00/
>>>> 
>>>> 
> 
> Kind regards,
> 
>>>> 
>>>> Patrick
>>>> 
>>> 
> 



More information about the net-dev mailing list