Call OnStarting before verifying response length (#1289) #1302
Conversation
| var writeException = await Assert.ThrowsAsync<ObjectDisposedException>(async () => | ||
| await response.Body.WriteAsync(Encoding.ASCII.GetBytes("Hello World"), 0, 11)); | ||
|
|
||
| var writeException = await Assert.ThrowsAsync<ObjectDisposedException>(async () => await response.Body.FlushAsync()); |
There was a problem hiding this comment.
Had to change so the check at
OnStarting as it did before, that method sees 0 bytes written and that's a mismatch against the old test which wrote 11 bytes).
There was a problem hiding this comment.
This seems bad. So you don't get a 500 anymore if you set a Content-Length?
There was a problem hiding this comment.
You do. The point here is to not only test that you get a 500 when an OnStarting callback throws, but also that the connection is not closed in that situation. Because the test code captures the exception from WriteAsync and does not rethrow it, we end up verifying the amount of bytes written after the app runs (see link above), which also leads to a 500 response but this one also closes the connection (because too few bytes were written and that will confuse the client).
Switching this to FlushAsync() makes the test simpler and avoids that situation. An alternative would be to keep the WriteAsync call and rethrow the exception, but this will result in more errors being logged and will require changing the assertion for that.
|
🔔 |
| } | ||
| else | ||
| { | ||
| VerifyAndUpdateWrite(data.Count); |
There was a problem hiding this comment.
Can we change InitializeResponse to not call VerifyAndUpdateWrite, and just call VerifyAndUpdateWrite afterwards?
There was a problem hiding this comment.
We need the check in InitializeResponse so we can return a 500 if the first write exceeds Content-Length. Without the check there a 200 response still started, despite the code failing later.
| httpContext.Response.ContentLength = 5; | ||
| await httpContext.Response.WriteAsync("hello, world"); | ||
| httpContext.Response.Body.Write(response, 0, response.Length); |
There was a problem hiding this comment.
The WriteAsync version of this test is just below. Need to verify both calls.
|
Updated. |
|
|
#1289
@Tratcher @halter73 @natemcmaster @pakrym