reimplement nextBackoffInterval tests to cover functional requirements#5161
reimplement nextBackoffInterval tests to cover functional requirements#5161
Conversation
| }) | ||
| } | ||
|
|
||
| func doNotCare[T any](x T) T { return x } |
There was a problem hiding this comment.
At first I was a bit confused; but I really like this! If we were using a property-based testing tool, this could be a randomly generated value.
There was a problem hiding this comment.
temporal/tests/testvars/test_vars.go
Line 211 in 1be76e3
There was a problem hiding this comment.
It shouldn't be in functional tests package but in some common test helpers package.
There was a problem hiding this comment.
And it needs to have Any for every type. I am going to improve that testvars package and will use it here too. One day.
There was a problem hiding this comment.
Unfortunately, I can't use Any(). There are some invariants that should hold for the values which I marked with doNotCare. At the same time, I want the reader to easily distinguish between values which can be changed without causing the test fail and the values which are important to the test success. Hope this makes sense.
tdeebswihart
left a comment
tdeebswihart
left a comment
There was a problem hiding this comment.
I vastly prefer how you structured these tests, thank you! I'm a big fan of test names that state the property being checked
| } | ||
|
|
||
| func Test_NextRetry(t *testing.T) { | ||
| a := assert.New(t) |
4 participants
What changed?
Reimplemented unit-tests for nextBackoffInterval
Why?
original tests descirption and functionality did not match, it what hard to say what it was testing. This is the best readability approach I could think of today. I tried table based approach, but I think it was less readable because call to the function under test was hidden.
How did you test it?
run tests
Potential risks
some edge cases were not covered by tests
Is hotfix candidate?
No