-
Notifications
You must be signed in to change notification settings - Fork 8.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
flush operation will overwrite the origin status code #1460
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1460 +/- ##
==========================================
+ Coverage 98.68% 98.68% +<.01%
==========================================
Files 36 36
Lines 1894 1895 +1
==========================================
+ Hits 1869 1870 +1
Misses 18 18
Partials 7 7
Continue to review full report at Codecov.
|
any feedback @thinkerou ? |
response_writer_test.go
Outdated
writer := &responseWriter{} | ||
writer.reset(w) | ||
|
||
writer.WriteHeader(500) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like http.StatusInternalServerError
response_writer_test.go
Outdated
// should return 500 | ||
resp, err := http.Get(testServer.URL) | ||
assert.NoError(t, err) | ||
assert.Equal(t, resp.StatusCode, 500) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use http.StatusInternalServerError
? and you should use assert.Equal(t, expect, actual)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you! done
please see my comment, a small problem, and then LGTM. |
4a6eea7
The status of responseWriter will be overwrite if flush was called. This is caused by the Flush of http.response.Flush().