Hi Pavel,
Thanks for the feedback. I've incorporated the changes you suggested,
and you can find them in the new webrev below.
http://cr.openjdk.java.net/~pconcannon/8217825/webrevs/webrev.02/
Kind regards,
Patrick
On 20/09/2019 13:21, Pavel Rappo wrote:
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.fu...@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
