Skip to content

Commit e9e7447

Browse files
GroxxGroxx
committed
fmt, removing globals from tests, scoping test logs (#1142)
* Removing some globals from tests * Use test-scoped loggers everywhere Conflicted with jwt-related things, so those have been removed. Otherwise no issues. Part of #1149
1 parent e2653f4 commit e9e7447

29 files changed

+477
-403
lines changed

‎Makefile‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ IMPORT_ROOT := go.uber.org/cadence
1010
THRIFT_GENDIR := .gen/go
1111
THRIFTRW_SRC := idls/thrift/cadence.thrift idls/thrift/shadower.thrift
1212
# one or more thriftrw-generated file(s), to create / depend on generated code
13-
THRIFTRW_OUT := $(THRIFT_GENDIR)/cadence/idl.go
13+
THRIFTRW_OUT := $(THRIFT_GENDIR)/cadence/cadence.go
1414
TEST_ARG ?= -v -race
1515

1616
# general build-product folder, cleaned as part of `make clean`

‎compatibility/thrift2proto.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,4 +36,4 @@ func NewThrift2ProtoAdapter(
3636
visibility apiv1.VisibilityAPIYARPCClient,
3737
) workflowserviceclient.Interface {
3838
return internal.NewThrift2ProtoAdapter(domain, workflow, worker, visibility)
39-
}
39+
}

‎evictiontest/workflow_cache_eviction_test.go‎

Lines changed: 22 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -44,9 +44,22 @@ import (
4444
"go.uber.org/cadence/internal/common"
4545
"go.uber.org/cadence/worker"
4646
"go.uber.org/yarpc"
47+
"go.uber.org/zap/zaptest"
4748
"golang.org/x/net/context"
4849
)
4950

51+
// copied from internal/test_helpers_test.go
52+
// this is the mock for yarpcCallOptions, as gomock requires the num of arguments to be the same.
53+
// see getYarpcCallOptions for the default case.
54+
func callOptions() []interface{} {
55+
return []interface{}{
56+
gomock.Any(), // library version
57+
gomock.Any(), // feature version
58+
gomock.Any(), // client name
59+
gomock.Any(), // feature flags
60+
}
61+
}
62+
5063
func testReplayWorkflow(ctx internal.Context) error {
5164
ao := internal.ActivityOptions{
5265
ScheduleToStartTimeout: time.Second,
@@ -86,9 +99,6 @@ func TestWorkersTestSuite(t *testing.T) {
8699
suite.Run(t, new(CacheEvictionSuite))
87100
}
88101

89-
// this is the mock for yarpcCallOptions, make sure length are the same
90-
var callOptions = []interface{}{gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()}
91-
92102
func createTestEventWorkflowExecutionStarted(eventID int64, attr *m.WorkflowExecutionStartedEventAttributes) *m.HistoryEvent {
93103
return &m.HistoryEvent{
94104
EventId: common.Int64Ptr(eventID),
@@ -149,25 +159,28 @@ func (s *CacheEvictionSuite) TestResetStickyOnEviction() {
149159
cacheSize := 5
150160
internal.SetStickyWorkflowCacheSize(cacheSize)
151161
// once for workflow worker because we disable activity worker
152-
s.service.EXPECT().DescribeDomain(gomock.Any(), gomock.Any(), callOptions...).Return(nil, nil).Times(1)
162+
s.service.EXPECT().DescribeDomain(gomock.Any(), gomock.Any(), callOptions()...).Return(nil, nil).Times(1)
153163
// feed our worker exactly *cacheSize* "legit" decision tasks
154164
// these are handcrafted decision tasks that are not blatantly obviously mocks
155165
// the goal is to trick our worker into thinking they are real so it
156166
// actually goes along with processing these and puts their execution in the cache.
157-
s.service.EXPECT().PollForDecisionTask(gomock.Any(), gomock.Any(), callOptions...).DoAndReturn(mockPollForDecisionTask).Times(cacheSize)
167+
s.service.EXPECT().PollForDecisionTask(gomock.Any(), gomock.Any(), callOptions()...).DoAndReturn(mockPollForDecisionTask).Times(cacheSize)
158168
// after *cacheSize* "legit" tasks are fed to our worker, start feeding our worker empty responses.
159169
// these will get tossed away immediately after polled, but we still need them so gomock doesn't compain about unexpected calls.
160170
// this is because our worker's poller doesn't stop, it keeps polling on the service client as long
161171
// as Stop() is not called on the worker
162-
s.service.EXPECT().PollForDecisionTask(gomock.Any(), gomock.Any(), callOptions...).Return(&m.PollForDecisionTaskResponse{}, nil).AnyTimes()
172+
s.service.EXPECT().PollForDecisionTask(gomock.Any(), gomock.Any(), callOptions()...).Return(&m.PollForDecisionTaskResponse{}, nil).AnyTimes()
163173
// this gets called after polled decision tasks are processed, any number of times doesn't matter
164-
s.service.EXPECT().RespondDecisionTaskCompleted(gomock.Any(), gomock.Any(), callOptions...).Return(&m.RespondDecisionTaskCompletedResponse{}, nil).AnyTimes()
174+
s.service.EXPECT().RespondDecisionTaskCompleted(gomock.Any(), gomock.Any(), callOptions()...).Return(&m.RespondDecisionTaskCompletedResponse{}, nil).AnyTimes()
165175
// this is the critical point of the test.
166176
// ResetSticky should be called exactly once because our workflow cache evicts when full
167177
// so if our worker puts *cacheSize* entries in the cache, it should evict exactly one
168-
s.service.EXPECT().ResetStickyTaskList(gomock.Any(), gomock.Any(), callOptions...).DoAndReturn(mockResetStickyTaskList).Times(1)
178+
s.service.EXPECT().ResetStickyTaskList(gomock.Any(), gomock.Any(), callOptions()...).DoAndReturn(mockResetStickyTaskList).Times(1)
169179

170-
workflowWorker := internal.NewWorker(s.service, "test-domain", "tasklist", worker.Options{DisableActivityWorker: true})
180+
workflowWorker := internal.NewWorker(s.service, "test-domain", "tasklist", worker.Options{
181+
DisableActivityWorker: true,
182+
Logger: zaptest.NewLogger(s.T()),
183+
})
171184
// this is an arbitrary workflow we use for this test
172185
// NOTE: a simple helloworld that doesn't execute an activity
173186
// won't work because the workflow will simply just complete

‎internal/activity_test.go‎

Lines changed: 21 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@ package internal
2222

2323
import (
2424
"context"
25-
"fmt"
2625
"testing"
2726

2827
"github.com/golang/mock/gomock"
@@ -54,46 +53,41 @@ func (s *activityTestSuite) TearDownTest() {
5453
s.mockCtrl.Finish() // assert mock’s expectations
5554
}
5655

57-
// this is the mock for yarpcCallOptions, make sure length are the same
58-
var callOptions = []interface{}{gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()}
59-
60-
var featureFlags = FeatureFlags{}
61-
6256
func (s *activityTestSuite) TestActivityHeartbeat() {
6357
ctx, cancel := context.WithCancel(context.Background())
64-
invoker := newServiceInvoker([]byte("task-token"), "identity", s.service, cancel, 1, make(chan struct{}), featureFlags)
58+
invoker := newServiceInvoker([]byte("task-token"), "identity", s.service, cancel, 1, make(chan struct{}), FeatureFlags{})
6559
ctx = context.WithValue(ctx, activityEnvContextKey, &activityEnvironment{serviceInvoker: invoker})
6660

67-
s.service.EXPECT().RecordActivityTaskHeartbeat(gomock.Any(), gomock.Any(), callOptions...).
61+
s.service.EXPECT().RecordActivityTaskHeartbeat(gomock.Any(), gomock.Any(), callOptions()...).
6862
Return(&shared.RecordActivityTaskHeartbeatResponse{}, nil).Times(1)
6963

7064
RecordActivityHeartbeat(ctx, "testDetails")
7165
}
7266

7367
func (s *activityTestSuite) TestActivityHeartbeat_InternalError() {
7468
ctx, cancel := context.WithCancel(context.Background())
75-
invoker := newServiceInvoker([]byte("task-token"), "identity", s.service, cancel, 1, make(chan struct{}), featureFlags)
69+
invoker := newServiceInvoker([]byte("task-token"), "identity", s.service, cancel, 1, make(chan struct{}), FeatureFlags{})
7670
ctx = context.WithValue(ctx, activityEnvContextKey, &activityEnvironment{
7771
serviceInvoker: invoker,
7872
logger: getTestLogger(s.T())})
7973

80-
s.service.EXPECT().RecordActivityTaskHeartbeat(gomock.Any(), gomock.Any(), callOptions...).
74+
s.service.EXPECT().RecordActivityTaskHeartbeat(gomock.Any(), gomock.Any(), callOptions()...).
8175
Return(nil, &shared.InternalServiceError{}).
8276
Do(func(ctx context.Context, request *shared.RecordActivityTaskHeartbeatRequest, opts ...yarpc.CallOption) {
83-
fmt.Println("MOCK RecordActivityTaskHeartbeat executed")
77+
s.T().Log("MOCK RecordActivityTaskHeartbeat executed")
8478
}).AnyTimes()
8579

8680
RecordActivityHeartbeat(ctx, "testDetails")
8781
}
8882

8983
func (s *activityTestSuite) TestActivityHeartbeat_CancelRequested() {
9084
ctx, cancel := context.WithCancel(context.Background())
91-
invoker := newServiceInvoker([]byte("task-token"), "identity", s.service, cancel, 1, make(chan struct{}), featureFlags)
85+
invoker := newServiceInvoker([]byte("task-token"), "identity", s.service, cancel, 1, make(chan struct{}), FeatureFlags{})
9286
ctx = context.WithValue(ctx, activityEnvContextKey, &activityEnvironment{
9387
serviceInvoker: invoker,
9488
logger: getTestLogger(s.T())})
9589

96-
s.service.EXPECT().RecordActivityTaskHeartbeat(gomock.Any(), gomock.Any(), callOptions...).
90+
s.service.EXPECT().RecordActivityTaskHeartbeat(gomock.Any(), gomock.Any(), callOptions()...).
9791
Return(&shared.RecordActivityTaskHeartbeatResponse{CancelRequested: common.BoolPtr(true)}, nil).Times(1)
9892

9993
RecordActivityHeartbeat(ctx, "testDetails")
@@ -103,12 +97,12 @@ func (s *activityTestSuite) TestActivityHeartbeat_CancelRequested() {
10397

10498
func (s *activityTestSuite) TestActivityHeartbeat_EntityNotExist() {
10599
ctx, cancel := context.WithCancel(context.Background())
106-
invoker := newServiceInvoker([]byte("task-token"), "identity", s.service, cancel, 1, make(chan struct{}), featureFlags)
100+
invoker := newServiceInvoker([]byte("task-token"), "identity", s.service, cancel, 1, make(chan struct{}), FeatureFlags{})
107101
ctx = context.WithValue(ctx, activityEnvContextKey, &activityEnvironment{
108102
serviceInvoker: invoker,
109103
logger: getTestLogger(s.T())})
110104

111-
s.service.EXPECT().RecordActivityTaskHeartbeat(gomock.Any(), gomock.Any(), callOptions...).
105+
s.service.EXPECT().RecordActivityTaskHeartbeat(gomock.Any(), gomock.Any(), callOptions()...).
112106
Return(&shared.RecordActivityTaskHeartbeatResponse{}, &shared.EntityNotExistsError{}).Times(1)
113107

114108
RecordActivityHeartbeat(ctx, "testDetails")
@@ -118,13 +112,13 @@ func (s *activityTestSuite) TestActivityHeartbeat_EntityNotExist() {
118112

119113
func (s *activityTestSuite) TestActivityHeartbeat_SuppressContinousInvokes() {
120114
ctx, cancel := context.WithCancel(context.Background())
121-
invoker := newServiceInvoker([]byte("task-token"), "identity", s.service, cancel, 2, make(chan struct{}), featureFlags)
115+
invoker := newServiceInvoker([]byte("task-token"), "identity", s.service, cancel, 2, make(chan struct{}), FeatureFlags{})
122116
ctx = context.WithValue(ctx, activityEnvContextKey, &activityEnvironment{
123117
serviceInvoker: invoker,
124118
logger: getTestLogger(s.T())})
125119

126120
// Multiple calls but only one call is made.
127-
s.service.EXPECT().RecordActivityTaskHeartbeat(gomock.Any(), gomock.Any(), callOptions...).
121+
s.service.EXPECT().RecordActivityTaskHeartbeat(gomock.Any(), gomock.Any(), callOptions()...).
128122
Return(&shared.RecordActivityTaskHeartbeatResponse{}, nil).Times(1)
129123
RecordActivityHeartbeat(ctx, "testDetails")
130124
RecordActivityHeartbeat(ctx, "testDetails")
@@ -133,11 +127,11 @@ func (s *activityTestSuite) TestActivityHeartbeat_SuppressContinousInvokes() {
133127

134128
// No HB timeout configured.
135129
service2 := workflowservicetest.NewMockClient(s.mockCtrl)
136-
invoker2 := newServiceInvoker([]byte("task-token"), "identity", service2, cancel, 0, make(chan struct{}), featureFlags)
130+
invoker2 := newServiceInvoker([]byte("task-token"), "identity", service2, cancel, 0, make(chan struct{}), FeatureFlags{})
137131
ctx = context.WithValue(ctx, activityEnvContextKey, &activityEnvironment{
138132
serviceInvoker: invoker2,
139133
logger: getTestLogger(s.T())})
140-
service2.EXPECT().RecordActivityTaskHeartbeat(gomock.Any(), gomock.Any(), callOptions...).
134+
service2.EXPECT().RecordActivityTaskHeartbeat(gomock.Any(), gomock.Any(), callOptions()...).
141135
Return(&shared.RecordActivityTaskHeartbeatResponse{}, nil).Times(1)
142136
RecordActivityHeartbeat(ctx, "testDetails")
143137
RecordActivityHeartbeat(ctx, "testDetails")
@@ -146,14 +140,14 @@ func (s *activityTestSuite) TestActivityHeartbeat_SuppressContinousInvokes() {
146140
// simulate batch picks before expiry.
147141
waitCh := make(chan struct{})
148142
service3 := workflowservicetest.NewMockClient(s.mockCtrl)
149-
invoker3 := newServiceInvoker([]byte("task-token"), "identity", service3, cancel, 2, make(chan struct{}), featureFlags)
143+
invoker3 := newServiceInvoker([]byte("task-token"), "identity", service3, cancel, 2, make(chan struct{}), FeatureFlags{})
150144
ctx = context.WithValue(ctx, activityEnvContextKey, &activityEnvironment{
151145
serviceInvoker: invoker3,
152146
logger: getTestLogger(s.T())})
153-
service3.EXPECT().RecordActivityTaskHeartbeat(gomock.Any(), gomock.Any(), callOptions...).
147+
service3.EXPECT().RecordActivityTaskHeartbeat(gomock.Any(), gomock.Any(), callOptions()...).
154148
Return(&shared.RecordActivityTaskHeartbeatResponse{}, nil).Times(1)
155149

156-
service3.EXPECT().RecordActivityTaskHeartbeat(gomock.Any(), gomock.Any(), callOptions...).
150+
service3.EXPECT().RecordActivityTaskHeartbeat(gomock.Any(), gomock.Any(), callOptions()...).
157151
Return(&shared.RecordActivityTaskHeartbeatResponse{}, nil).
158152
Do(func(ctx context.Context, request *shared.RecordActivityTaskHeartbeatRequest, opts ...yarpc.CallOption) {
159153
ev := newEncodedValues(request.Details, nil)
@@ -176,13 +170,13 @@ func (s *activityTestSuite) TestActivityHeartbeat_SuppressContinousInvokes() {
176170
// simulate batch picks before expiry, with out any progress specified.
177171
waitCh2 := make(chan struct{})
178172
service4 := workflowservicetest.NewMockClient(s.mockCtrl)
179-
invoker4 := newServiceInvoker([]byte("task-token"), "identity", service4, cancel, 2, make(chan struct{}), featureFlags)
173+
invoker4 := newServiceInvoker([]byte("task-token"), "identity", service4, cancel, 2, make(chan struct{}), FeatureFlags{})
180174
ctx = context.WithValue(ctx, activityEnvContextKey, &activityEnvironment{
181175
serviceInvoker: invoker4,
182176
logger: getTestLogger(s.T())})
183-
service4.EXPECT().RecordActivityTaskHeartbeat(gomock.Any(), gomock.Any(), callOptions...).
177+
service4.EXPECT().RecordActivityTaskHeartbeat(gomock.Any(), gomock.Any(), callOptions()...).
184178
Return(&shared.RecordActivityTaskHeartbeatResponse{}, nil).Times(1)
185-
service4.EXPECT().RecordActivityTaskHeartbeat(gomock.Any(), gomock.Any(), callOptions...).
179+
service4.EXPECT().RecordActivityTaskHeartbeat(gomock.Any(), gomock.Any(), callOptions()...).
186180
Return(&shared.RecordActivityTaskHeartbeatResponse{}, nil).
187181
Do(func(ctx context.Context, request *shared.RecordActivityTaskHeartbeatRequest, opts ...yarpc.CallOption) {
188182
require.Nil(s.T(), request.Details)
@@ -200,14 +194,14 @@ func (s *activityTestSuite) TestActivityHeartbeat_SuppressContinousInvokes() {
200194
func (s *activityTestSuite) TestActivityHeartbeat_WorkerStop() {
201195
ctx, cancel := context.WithCancel(context.Background())
202196
workerStopChannel := make(chan struct{})
203-
invoker := newServiceInvoker([]byte("task-token"), "identity", s.service, cancel, 5, workerStopChannel, featureFlags)
197+
invoker := newServiceInvoker([]byte("task-token"), "identity", s.service, cancel, 5, workerStopChannel, FeatureFlags{})
204198
ctx = context.WithValue(ctx, activityEnvContextKey, &activityEnvironment{serviceInvoker: invoker})
205199

206200
heartBeatDetail := "testDetails"
207201
waitCh := make(chan struct{}, 1)
208202
waitCh <- struct{}{}
209203
waitC2 := make(chan struct{}, 1)
210-
s.service.EXPECT().RecordActivityTaskHeartbeat(gomock.Any(), gomock.Any(), callOptions...).
204+
s.service.EXPECT().RecordActivityTaskHeartbeat(gomock.Any(), gomock.Any(), callOptions()...).
211205
Return(&shared.RecordActivityTaskHeartbeatResponse{}, nil).
212206
Do(func(ctx context.Context, request *shared.RecordActivityTaskHeartbeatRequest, opts ...yarpc.CallOption) {
213207
if _, ok := <-waitCh; ok {

‎internal/client.go‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -542,7 +542,6 @@ func NewClient(service workflowserviceclient.Interface, domain string, options *
542542
} else {
543543
tracer = opentracing.NoopTracer{}
544544
}
545-
546545
return &workflowClient{
547546
workflowService: metrics.NewWorkflowServiceWrapper(service, metricScope),
548547
domain: domain,
@@ -569,7 +568,6 @@ func NewDomainClient(service workflowserviceclient.Interface, options *ClientOpt
569568
metricScope = options.MetricsScope
570569
}
571570
metricScope = tagScope(metricScope, tagDomain, "domain-client", clientImplHeaderName, clientImplHeaderValue)
572-
573571
return &domainClient{
574572
workflowService: metrics.NewWorkflowServiceWrapper(service, metricScope),
575573
metricsScope: metricScope,

‎internal/context_test.go‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@ import (
2626
"time"
2727

2828
"github.com/stretchr/testify/assert"
29-
"go.uber.org/zap/zaptest"
3029
)
3130

3231
func TestContextChildParentCancelRace(t *testing.T) {
@@ -81,9 +80,7 @@ func TestContextConcurrentCancelRace(t *testing.T) {
8180
In principle this must be safe to do - contexts are supposed to be concurrency-safe. Even if ours are not actually
8281
safe (for valid reasons), our execution model needs to ensure they *act* like it's safe.
8382
*/
84-
s := WorkflowTestSuite{}
85-
s.SetLogger(zaptest.NewLogger(t))
86-
env := s.NewTestWorkflowEnvironment()
83+
env := newTestWorkflowEnv(t)
8784
wf := func(ctx Context) error {
8885
ctx, cancel := WithCancel(ctx)
8986
racyCancel := func(ctx Context) {

0 commit comments

Comments
 (0)