Added test for pinot_visibility_metric_clients.go #5767
Conversation
bowenxia
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
Groxx,
shijiesheng,
agautam478,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
Codecov Report
Additional details and impacted filessee 8 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Pull Request Test Coverage Report for Build 018e2026-edf4-4412-9bff-53e48e77108bDetails
💛 - Coveralls |
| mockMetricClient.On("Scope", mock.Anything, mock.Anything).Return(mockScope).Once() | ||
| mockScope.On("StartTimer", mock.Anything, mock.Anything).Return(testStopwatch).Once() | ||
|
|
||
| if test.expectedError != nil { |
There was a problem hiding this comment.
this is probably ok, but in general if we see that we're having to branch out at all in the table-test Run method that's a sign that we should probably use a different pattern.
A nice way to do this (I think), is to create an affordance - or a function which takes the mock and sets the expectations per Table-test row. Eg:
tests := map[string]struct {
request *p.VisibilityDeleteWorkflowExecutionRequest
producerMockAffordance function(*MockPublisher)
expectedError error
}{
"Case1: error case": {
request: errorRequest,
producerMockAffordance: func(m MockPublisher){
m.Expect().Publish(...).Return(errors.New("an error"))
}
expectedError: fmt.Errorf("error"),
}
}
for name, td := range tests
t.Run(name, func(t testing.T) {
... create mocks
td.producerAffordance(mockProducer)
// assertions
}
}
There was a problem hiding this comment.
Good idea. Will make another PR to do this.
| // create mock clients | ||
| ctrl := gomock.NewController(t) | ||
| mockPinotClient := pnt.NewMockGenericClient(ctrl) | ||
| mockProducer := &mocks.KafkaProducer{} | ||
| mockMetricClient := &metricsClientMocks.Client{} | ||
| mockScope := &metricsClientMocks.Scope{} | ||
|
|
||
| // create metricClient | ||
| logger := testlogger.New(t) | ||
| mgr := NewPinotVisibilityStore(mockPinotClient, &service.Config{ | ||
| ValidSearchAttributes: dynamicconfig.GetMapPropertyFn(definition.GetDefaultIndexedKeys()), | ||
| ESIndexMaxResultWindow: dynamicconfig.GetIntPropertyFn(3), | ||
| }, mockProducer, testlogger.New(t)) | ||
| visibilityStore := mgr.(*pinotVisibilityStore) | ||
| pinotVisibilityManager := p.NewVisibilityManagerImpl(visibilityStore, logger) | ||
| visibilityMgr := NewPinotVisibilityMetricsClient(pinotVisibilityManager, mockMetricClient, logger) | ||
| metricsClient := visibilityMgr.(*pinotVisibilityMetricsClient) | ||
|
|
||
| // mock behaviors | ||
| mockMetricClient.On("Scope", mock.Anything, mock.Anything).Return(mockScope).Once() | ||
| mockScope.On("StartTimer", mock.Anything, mock.Anything).Return(testStopwatch).Once() |
There was a problem hiding this comment.
This initialisation part looks pretty much the same across all 17 tests which is a big overhead.
Is it possible to extract it into some function?
This looks like a great candidate to Suite. I know we're trying to avoid it, but we can maybe borrow the idea of TestSuite' SetupTest().
There was a problem hiding this comment.
Moving the absolutely-fixed parts to helper functions make sense but I'd be cautious of doing that because you may end up passing bunch of flags/parameters to make such setup helper functions handle all cases. Some level of redundancy in test code is OK and readability/self-containedness/explicitness/repeatability should be optimized IMO.
Suites work fine until they don't :) When you want to add a new test cases that initializes common components slightly differently you end up doing hacky things (example) or add the redundant code at that point anyway.
There was a problem hiding this comment.
I would second Taylan's point that it's hard to generalize assertions in a single place, but I think you're right that this can and perhaps should be less verbose, and, to your wider point, that we need to be careful to ensure the tests remain high-quality as we sprint for coverage.
I didn't see it too concerning, but in general I'd err towards avoiding if/else blocks with branching mock assertions such as line 109/116 and subsequently. If the assertions are different between table test invocations, then probably best to set them per table-row (with a func which is invoked to setup the mocks per row). That would make it easier to follow I think.
What changed?
Added test for pinot_visibility_metric_clients.go
Why?
for a better code coverage: (100%)
How did you test it?
unit test
Potential risks
Release notes
Documentation Changes