-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Error checking on unit tests #2959
Conversation
Arguments are evaluated immediately on defer calls
Codecov Report
@@ Coverage Diff @@
## main #2959 +/- ##
=======================================
Coverage 91.62% 91.62%
=======================================
Files 312 312
Lines 15445 15445
=======================================
Hits 14151 14151
Misses 884 884
Partials 410 410 Continue to review full report at Codecov.
|
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.
Most of the changes are adding a require.NoError
call with some error, I left a comment for a couple of them in which I am not super sure
receiver/otlpreceiver/otlp_test.go
Outdated
@@ -802,7 +802,7 @@ func TestShutdown(t *testing.T) { | |||
client := &http.Client{} | |||
resp, err2 := client.Do(req) | |||
if err2 == nil { | |||
ioutil.ReadAll(resp.Body) | |||
ioutil.ReadAll(resp.Body) // nolint:errcheck |
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 am not sure what's the point of this since the output is not used
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.
Based on the documentation https://golang.org/pkg/net/http/ the Body needs to be closed, but you don't have to read all.
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 can replace with a resp.Body.Close()
call then
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.
Done.
@@ -88,7 +88,7 @@ func TestZipkinExporter_roundtripJSON(t *testing.T) { | |||
require.NotNil(t, zi) | |||
|
|||
require.NoError(t, zi.Start(context.Background(), componenttest.NewNopHost())) | |||
defer zi.Shutdown(context.Background()) | |||
defer func() { require.NoError(t, zi.Shutdown(context.Background())) }() |
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.
Everywhere in the PR :)
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.
👍, makes sense, I can search and replace.
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.
Done.
receiver/otlpreceiver/otlp_test.go
Outdated
@@ -802,7 +802,7 @@ func TestShutdown(t *testing.T) { | |||
client := &http.Client{} | |||
resp, err2 := client.Do(req) | |||
if err2 == nil { | |||
ioutil.ReadAll(resp.Body) | |||
ioutil.ReadAll(resp.Body) // nolint:errcheck |
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.
Based on the documentation https://golang.org/pkg/net/http/ the Body needs to be closed, but you don't have to read all.
…ry#2959) - now supports metrics datatype. - also supports filtering by name and attributes (metrics only) - query URL has changed from `/` to `/spans` and `/metrics` - fixed a number of other subtle bugs/issues
Description:
Fix errcheck errors on test code, counterpart to #2881.
Link to tracking Issue: Updates #2789
To be tracked on #2819