[code-coverage] update admin and frontend client to use generated code #5702
Conversation
ketsiambaku
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
Groxx,
shijiesheng,
agautam478,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
client/admin/interface.go
Outdated
| @@ -34,6 +34,7 @@ import ( | |||
| //go:generate gowrap gen -g -p . -i Client -t ../templates/errorinjectors.tmpl -o ../wrappers/errorinjectors/admin_generated.go -v client=Admin | |||
| //go:generate gowrap gen -g -p . -i Client -t ../templates/grpc.tmpl -o ../wrappers/grpc/admin_generated.go -v client=Admin -v package=adminv1 -v path=github.com/uber/cadence-idl/go/proto/admin/v1 -v prefix=Admin | |||
| //go:generate gowrap gen -g -p . -i Client -t ../templates/thrift.tmpl -o ../wrappers/thrift/admin_generated.go -v client=Admin -v prefix=Admin | |||
| //go:generate gowrap gen -g -p . -i Client -t ../templates/tchannel.tmpl -o ../wrappers/tchannel/admin_generated.go -v client=Admin | |||
There was a problem hiding this comment.
Could you elaborate on why it is tchannel? :)
As far as I understand, it is an abstraction around thrift or grpc clients.
Not sure what would be the best name here.
timed? timed_out?
There was a problem hiding this comment.
The naming was based off of this comment under the constructors. However this is old code maybe previously used as a tchannel wrapper and was refactored later. I like timed better
There was a problem hiding this comment.
As far as I see @shijiesheng called it timeout wrapper in #5728
Lets rename it to timeout then.
ketsiambaku
changed the title
Pull Request Test Coverage Report for Build 018e1d74-4044-43d0-a74b-6220b624b802Details
💛 - Coveralls |
client/templates/timed.tmpl
Outdated
| ctx, cancel := c.createContext({{(index $method.Params 0).Name}}) | ||
| {{- end -}} | ||
| {{- else if eq $ClientName "Frontend" -}} | ||
| {{- if or (eq $method.Name "ListArchivedWorkflowExecutions") |
There was a problem hiding this comment.
It is hard to read/add more stuff into if cases.
@Zijian-Uber Introduced a nice way to form exceptions/special handing with template list, see https://github.com/uber/cadence/blob/master/service/frontend/templates/clusterredirection.tmpl#L17
Then you can simplify the condition significantly:
https://github.com/uber/cadence/blob/master/service/frontend/templates/clusterredirection.tmpl#L62
ketsiambaku
force-pushed
the
gowrap-client-frontend
branch
from
318cbdf
to
6a12970
Compare
ketsiambaku
force-pushed
the
gowrap-client-frontend
branch
from
6a12970
to
ca7cbd7
Compare
Codecov Report
Additional details and impacted files
... and 4 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| "context" | ||
| "time" | ||
| "context" | ||
| "time" |
There was a problem hiding this comment.
it's relatively minor but: see if you can switch your IDE to use tabs instead of spaces in *.tmpl files? fewer changes.
ketsiambaku
force-pushed
the
gowrap-client-frontend
branch
from
ab40a0f
to
deb140c
Compare
| FrontendDefaultLongPollTimeout = time.Minute * 3 | ||
| ) | ||
|
|
||
| func createContext(parent context.Context, timeout time.Duration) (context.Context, context.CancelFunc) { |
There was a problem hiding this comment.
Since all client decorators are using the same logic to create context, I removed this from the template and into a function that they can all share
What changed?
In this PR admin & frontend client wrapper is generated using gowrap
history was generated in a previous PR #5728 however, I am adding to/modifiing the template so it can generate code for the other decorators as well.
Will have an follow up PR for matching client because of the multiple special cases :)
Why?
To improve code coverage
To reduce manual work when new methods are added in the future
How did you test it?
unit tests
Potential risks
Release notes
Documentation Changes