[history] fix generated timeout wrapper #5737
Conversation
shijiesheng
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
Groxx,
agautam478,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
| select { | ||
| case <-ctx.Done(): | ||
| return nil, ctx.Err() | ||
| case <-time.After(time.Millisecond * 100): |
There was a problem hiding this comment.
can we somehow use mock time source to avoid relying on time passed? this can cause flakiness
There was a problem hiding this comment.
I tried. It's very complicated. The real issue here is incorrect test case with timeout set as 100. A quicker fix would be changing the timeout to 150ms and that should kill the flakiness
There was a problem hiding this comment.
yeah let's go with higher number then
Codecov Report
Additional details and impacted filessee 6 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| @@ -3,6 +3,7 @@ import ( | |||
| "time" | |||
| ) | |||
|
|
|||
| {{$exclude := splitList "|" (index .Vars "exclude")}} | |||
There was a problem hiding this comment.
I guess we're hitting the limitations of this templating
There was a problem hiding this comment.
Yes, but it's still useful to generate for most handlers.
There was a problem hiding this comment.
yeah, no concerns, just musing
| t.Run("no timeout", func(t *testing.T) { | ||
| m := history.NewMockClient(gomock.NewController(t)) | ||
| m.EXPECT().GetReplicationMessages(gomock.Any(), gomock.Any()).DoAndReturn(func(ctx context.Context, r *types.GetReplicationMessagesRequest, opt ...yarpc.CallOption) (*types.GetReplicationMessagesResponse, error) { | ||
| for { |
There was a problem hiding this comment.
sorry, maybe just me being dumb, but I am not sure I understand why this is a loop?
There was a problem hiding this comment.
It's for wait and handle the context timeout.
There was a problem hiding this comment.
It doesn't matter very much, so this is more a nit comment, but wouldn't jsut the select statement work? Or am I just confused somehow
There was a problem hiding this comment.
I think you are right. We don't have default case so it's not really needed.
shijiesheng
force-pushed
the
history-client-fix
branch
from
c714e5a
to
ebe1134
Compare
shijiesheng
force-pushed
the
history-client-fix
branch
from
7b63920
to
0cce260
Compare
Pull Request Test Coverage Report for Build 018e1a72-0b04-4c09-9b2a-83350045a3cdDetails
💛 - Coveralls |
What changed?
#5728 code generated history client should exclude some endpoints.
Why?
Some replication endpoints don't have timeout before
How did you test it?
unit test
Potential risks
Release notes
Documentation Changes