TestHost: support WebSocket testing #336
Conversation
|
Hi @tmds, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! The agreement was validated by .NET Foundation and real humans are currently evaluating your PR. TTYL, DNFBOT; |
|
|
||
| public async Task<WebSocket> ConnectAsync(Uri uri, CancellationToken cancellationToken) | ||
| { | ||
| HttpClient client = _testServer.CreateClient(); |
There was a problem hiding this comment.
I think the WebSocketClient can bypass the HttpClient entirely. HttpClient doesn't have any concept of WebSockets, so tacking them on is pretty awkward. Instead of creating an HttpRequestMessage here just create an IFeatureCollection & DefaultHttpContext directly (This may be a little easer after aspnet/HttpAbstractions#367).
… on Microsoft.AspNet.WebSockets
|
@Tratcher I re-implemented the feature based on your feedback |
| @@ -66,22 +66,22 @@ public ClientHandler([NotNull] Func<IFeatureCollection, Task> next, PathString p | |||
|
|
|||
| // Async offload, don't let the test code block the caller. | |||
| var offload = Task.Factory.StartNew(async () => | |||
| { | |||
There was a problem hiding this comment.
You can revert this file if only the formatting has changed.
|
This looks much better overall. It's definitely going to need some tests though. |
|
|
||
| Task<System.Net.WebSockets.WebSocket> IHttpWebSocketFeature.AcceptAsync(WebSocketAcceptContext context) | ||
| { | ||
| var websockets = WebSocket.CreatePair(context.SubProtocol); |
There was a problem hiding this comment.
Store both of these and abort/dispose them when _next finishes or throws (e.g. PipelineComplete or PipelineFailed).
- WebSocket: rename to TestWebSocket to avoid name clash with System.Net.WebSockets.WebSocket - TestWebSocket: fully implement Close/Abort/Dispose logic - TestClientTests: add tests for read/write, dispose and endofmessage - WebSocketClient: dispose server websocket when pipeline ends - WebSocketClient: immediately construct HttpRequest, drop HttpRequestMessage
|
@Tratcher, @davidfowl, I improved the code further based on your comments
|
|
|
||
| namespace Microsoft.AspNet.TestHost | ||
| { | ||
| internal class TestWebSocket : System.Net.WebSockets.WebSocket |
There was a problem hiding this comment.
Do you still need the full type name here? System.Net.WebSockets.WebSocket
| @@ -7,6 +7,10 @@ | |||
| using Microsoft.AspNet.Builder; | |||
| using Microsoft.AspNet.Http; | |||
| using Xunit; | |||
| using System.Threading; | |||
|
@Tratcher, is this ready for merging? |
|
@tmds Sorry, this is on hold while we resolve an internal discussion about WebSockets. We'll get back to you soon. cc: @DamianEdwards |
|
@Tratcher no problem, take your time |
|
|
I'll try rebasing myself.
|
Rebased and merged. |
Implement #315