Add unit tests for DurationToXXX and XXXToDuration #5530
Conversation
arzonus
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
Groxx,
shijiesheng,
agautam478,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
| // DurationToSeconds converts time.Duration to number of seconds | ||
| func DurationToSeconds(d time.Duration) int64 { | ||
| return int64(d / time.Second) | ||
| } | ||
|
|
||
| // DurationToMilliseconds converts time.Duration to number of milliseconds | ||
| func DurationToMilliseconds(d time.Duration) int64 { |
There was a problem hiding this comment.
Hmmm. I we can create a generic function:
func DurationToUnit(d time.Duration, unit time.Duration) int64{
return int64(d / unit)
}
There was a problem hiding this comment.
you can go a bit crazier and do
https://go.dev/play/p/syBQijmw6OW
for both functions :)
There was a problem hiding this comment.
that'll be the first instance of Generics in the repo probably. Still, we're at 1.20 now, so should be ok
| // DaysToDuration converts number of 24 hour days to time.Duration | ||
| func DaysToDuration(d int32) time.Duration { | ||
| return time.Duration(d) * (24 * time.Hour) | ||
| } | ||
|
|
||
| // HoursToDuration converts number of hours to time.Duration | ||
| func HoursToDuration(d int64) time.Duration { |
There was a problem hiding this comment.
The same here
// ToDuration converts a number of units to time.Duration with the specified unit
func ToDuration(value int64, unit time.Duration) time.Duration {
return time.Duration(value) * unit
}
| @@ -693,3 +693,58 @@ func TestValidateRetryPolicy_Error(t *testing.T) { | |||
| }) | |||
| } | |||
| } | |||
|
|
|||
| func TestDurationToDays(t *testing.T) { | |||
There was a problem hiding this comment.
Then you can test generic implementation.
…tests_for_duration_to_duration # Conflicts: # common/util_test.go
|
@3vilhamster I understand your suggestions, but it will not improve the coverage, because it will still require us to write tests calling these functions, not only the generic function. If we're going to replace these functions with a generic function it will require replacing it in all places. Also, there is one change because |
|
If you don't want to introduce big changes, you can introduce this functions, test them separately and make existing wrappers call them. Also, you can add annotation that functions are deprecated and we propose usage of generic version. |
…tests_for_duration_to_duration # Conflicts: # common/util_test.go
…tests_for_duration_to_duration # Conflicts: # common/util_test.go
|
I am not sure that we should use a generic function here. The goal of the PR is to improve test coverage. If we change the internals of these functions to the generic function, we still need to have tests to verify that these changes are correct and backward compatible. Of course, a 1-line function shouldn't cause bugs there, but it's better to be sure that these changes don't break something. So, I'd like to leave it as it is and don't use the generic function here right now. But I agree that it makes to use the generic function if we're going to use these functions somewhere additionally. WDYT? @3vilhamster @davidporter-id-au @taylanisikdemir |
…tests_for_duration_to_duration # Conflicts: # common/util_test.go
Pull Request Test Coverage Report for Build 018ced49-ccee-4365-bef4-c0d3488c6bff
💛 - Coveralls |
What changed?
Non-used DurationToXXX and XXXToDuration have been removed. Unit tests have been added for others.
Why?
The funcs were not covered by unit tests
How did you test it?
Unit tests are passed
Potential risks
Release notes
Documentation Changes