Conversation
Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>
Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>
Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>
Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5627 +/- ##
==========================================
+ Coverage 26.54% 26.57% +0.02%
==========================================
Files 477 477
Lines 50666 50644 -22
==========================================
+ Hits 13450 13458 +8
+ Misses 36153 36128 -25
+ Partials 1063 1058 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| func (r *ToolRegistry) newTmpDir() (string, error) { | ||
| return os.MkdirTemp(r.tmpDir, "") | ||
| type fakeClient struct { | ||
| pipedservicetest.MockPluginServiceClient |
There was a problem hiding this comment.
Use it to reduce the cost of maintaining fake implementation.
There was a problem hiding this comment.
[IMO]
Using gomock only for this embedding is a bit complex.
How about embedding service.PluginServiceClient instead of generated mock?
There was a problem hiding this comment.
Thanks, sounds good!
I thought we needed the actual instance to override the specific method. 🙏
It works fine.
Warashi
left a comment
Warashi
left a comment
There was a problem hiding this comment.
I agree to use fake in the toolregistrytest.
I commented on how we prepare the fake.
| func (r *ToolRegistry) newTmpDir() (string, error) { | ||
| return os.MkdirTemp(r.tmpDir, "") | ||
| type fakeClient struct { | ||
| pipedservicetest.MockPluginServiceClient |
There was a problem hiding this comment.
[IMO]
Using gomock only for this embedding is a bit complex.
How about embedding service.PluginServiceClient instead of generated mock?
Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>
3 participants
What this PR does:
as title
Why we need it:
It is better to use the same type in both the cases used in actual logic and test like
zaptest.https://github.com/uber-go/zap/blob/master/zaptest/logger.go#L77
The users don't need to define an additional interface to replace the tool registry on the test.
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: