fix: request body logging [#84] #94
Conversation
|
@flash286 thank you for the pull request! The approach looks good. I would like to investigate a bit more before merging. In particular, look at the integrations that use FromHTTPRequest -- it may be the case that the body was read and is not available anymore when we want to report an error. Will give more review feedback tomorrow, wanted to keep you posted ;) Once again thank you! |
There was a problem hiding this comment.
@flash286 thanks for your contribution! Special awesomeness medal 🥇 for adding tests!
I'd like @kamilogorek to have a look at this too as he may have some insights.
interfaces_test.go
Outdated
|
|
||
| const testPayload = `{"test_data": true}` | ||
|
|
||
| func TestRequest_FromHTTPRequest(t *testing.T) { |
There was a problem hiding this comment.
It's awesome to see tests for this, thank you very much!!! ❤️
We follow the Go MixedCaps naming convention, could you please update the function name to TestRequestFromHTTPRequest?
interfaces_test.go
Outdated
| sentryRequest := Request{} | ||
| sentryRequest = sentryRequest.FromHTTPRequest(req) | ||
| assertEqual(t, sentryRequest.Data, testPayload) | ||
| }) |
There was a problem hiding this comment.
The body of the first test is exactly the same as the early portion of the second test.
Since the assert* helpers call t.Error, execution continues even after an "assertion error". In that case, I think it is enough to have just the second test.
There was a problem hiding this comment.
I Agree, second one should cover all we need
interfaces_test.go
Outdated
| "testing" | ||
| ) | ||
|
|
||
| const testPayload = `{"test_data": true}` |
There was a problem hiding this comment.
Since this is not used anywhere else and to avoid polluting the global namespace for tests, how about we move this inside of the test method?
|
Agree with all your comments, nice review! |
There was a problem hiding this comment.
@flash286 thanks for the update! I noticed one opportunity to capture the body bytes even when there's an error. Would you like to tackle it in this PR? Extra bonus if that includes a unit test 😄
Otherwise let's merge this in and cut an improvement issue.
|
@flash286 thank you very much for your contribution! |
fix: request body logging [#84]