Conversation
stephanos
force-pushed
the
stephanos/versioning-eventuallywitht
branch
from
327c95b to
76d568a
Compare
stephanos
changed the title
stephanos
force-pushed
the
stephanos/versioning-eventuallywitht
branch
from
327c95b to
9834f54
Compare
| } | ||
| return true | ||
| }, 10*time.Second, 100*time.Millisecond) | ||
| return true |
There was a problem hiding this comment.
hm, why was this nested?
stephanos
force-pushed
the
stephanos/versioning-eventuallywitht
branch
4 times, most recently
from
8ef7b41 to
2a6fece
Compare
| ).Error()) | ||
| } | ||
| s.NoError(err) | ||
| return err == nil |
There was a problem hiding this comment.
I don't understand why this was so complicated when in the end it really retries until there are no errors anymore?
There was a problem hiding this comment.
I think the point here is to retry only if you do get an error that is one of notFound or ErrCurrentVersionDoesNotHaveAllTaskQueues
If this API were to return something else, then the s.NoError(err) would fail and the test would fail imo
There was a problem hiding this comment.
Ohh, I see. Alright, the s.NoError did fail the test before my change, but only after the entire Eventually timeout elapsed. I've restored the original intent in 381d35d while not waiting for the entire timeout anymore.
stephanos
force-pushed
the
stephanos/versioning-eventuallywitht
branch
from
2a6fece to
d2ccd90
Compare
stephanos
force-pushed
the
stephanos/versioning-eventuallywitht
branch
from
d2ccd90 to
df03fed
Compare
Shivs11
left a comment
Shivs11
left a comment
There was a problem hiding this comment.
just left that one comment since i think your change changes the behaviour here
| TaskQueueType: enumspb.TASK_QUEUE_TYPE_WORKFLOW, | ||
| }, | ||
| }) | ||
| s.Require().NoError(err) |
There was a problem hiding this comment.
Also removed a few s.Require(). which are unnecessary
2 participants
What changed?
Replaces (almost) all use of
s.Eventuallywiths.EventuallyWithT.Why?
Assertions are often used inside
s.Eventuallyhere and that's not safe as it aborts the test immediately.