-
Notifications
You must be signed in to change notification settings - Fork 2.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
Added tests for the go/trace package #15052
Conversation
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #15052 +/- ##
==========================================
+ Coverage 47.29% 47.74% +0.44%
==========================================
Files 1137 1155 +18
Lines 238684 240231 +1547
==========================================
+ Hits 112895 114701 +1806
+ Misses 117168 116926 -242
+ Partials 8621 8604 -17 ☔ View full report in Codecov by Sentry. |
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.
The tests that have been added are failing when running with -race
flag. We need to fix this before we can merge the PR, since the CI action unit_race
will continue to fail otherwise.
f0dd97e
to
c898b5b
Compare
Fixed it. |
go/trace/logger_test.go
Outdated
logger := traceLogger{} | ||
|
||
// Redirect stderr to a buffer | ||
rescueStderr := os.Stderr | ||
r, w, _ := os.Pipe() | ||
os.Stderr = w | ||
|
||
want := "This is an error message" | ||
logger.Error(want) | ||
|
||
w.Close() | ||
got, _ := io.ReadAll(r) | ||
os.Stderr = rescueStderr | ||
|
||
assert.Contains(t, string(got), want) | ||
|
||
r, w, _ = os.Pipe() | ||
os.Stderr = w | ||
|
||
want = "This is an log message" | ||
logger.Log(want) | ||
|
||
w.Close() | ||
got, _ = io.ReadAll(r) | ||
os.Stderr = rescueStderr | ||
|
||
assert.Contains(t, string(got), want) |
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.
Instead of having such a long function, it is better to make test cases, that will make the setup code a lot smaller, since it will be shared. Also the test is far more readable this way, since the expecatation and the parameters are listed together.
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. Thanks for the advice.
go/trace/plugin_datadog_test.go
Outdated
func TestDdCloser(t *testing.T) { | ||
dc := ddCloser{} | ||
require.NoError(t, dc.Close()) | ||
} |
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.
This test isn't useful
go/trace/utils_test.go
Outdated
logFunc := LogErrorsWhenClosing(&fakeCloser{}) | ||
|
||
// Redirect stderr to a buffer | ||
rescueStderr := os.Stderr |
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.
Instead of setting os.Stderr back to rescueStderr, just do it once in a defer call.
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.
This time, I created a helper function for this task and used t.Cleanup
to reset the original values.
Signed-off-by: VaibhavMalik4187 <[email protected]>
c898b5b
to
836378e
Compare
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.
LGTM now
Description
Increases the code coverage of the trace package to >42%.
Related Issue(s)
#14931
Checklist