-
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
tests: add tests for go/protoutil/duration
#14965
Conversation
Signed-off-by: Manik Rana <[email protected]>
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
|
/go/protoutil/duration
go/protoutil/duration
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14965 +/- ##
==========================================
- Coverage 47.26% 47.25% -0.01%
==========================================
Files 1138 1138
Lines 238842 238842
==========================================
- Hits 112880 112869 -11
- Misses 117368 117376 +8
- Partials 8594 8597 +3 ☔ View full report in Codecov by Sentry. |
Code coverage report: |
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, thank you for this contribution! However there is still one error in duration.go
which is not tested:
if (d < 0) != (dpb.Nanos < 0) {
return 0, true, fmt.Errorf("duration: %v is out of range for time.Duration", dpb)
}
Do you think you can test this condition as well?
@frouioui yes, I'm aware.
Sure thing, I'll try my best |
go/protoutil/duration_test.go
Outdated
@@ -80,3 +90,40 @@ func TestDurationFromProto(t *testing.T) { | |||
}) | |||
} | |||
} | |||
|
|||
func TestDurationToProto(t *testing.T) { | |||
t.Parallel() |
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 think the t.Parallel()
is cargo culted here from the existing tests, but these tests are so fast that I think we should just drop it everywhere.
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.
Removed t.Parallel()
from this file
Signed-off-by: Manik Rana <[email protected]>
Signed-off-by: Manik Rana <[email protected]>
@frouioui done |
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, thank you!
Description
Adds tests for
DurationToProto
and ananoseconds
testcase forDurationFromProto
Related Issue(s)
#14931
Checklist
Deployment Notes