Skip to content

Commit 9ec9816

Browse files
committed
[common] Introduce testlogger as a workaround of poor lifecycle
1 parent 7a3beaa commit 9ec9816

18 files changed

+376
-60
lines changed

internal/activity_task_handler_test.go

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -27,14 +27,15 @@ import (
2727
"testing"
2828
"time"
2929

30+
"go.uber.org/cadence/internal/common/testlogger"
31+
3032
"github.com/jonboulle/clockwork"
3133

3234
"github.com/golang/mock/gomock"
3335
"github.com/opentracing/opentracing-go"
3436
"github.com/pborman/uuid"
3537
"github.com/stretchr/testify/assert"
3638
"github.com/stretchr/testify/require"
37-
"go.uber.org/zap/zaptest"
3839

3940
"go.uber.org/cadence/.gen/go/cadence/workflowservicetest"
4041
s "go.uber.org/cadence/.gen/go/shared"
@@ -57,7 +58,7 @@ func TestActivityTaskHandler_Execute_deadline(t *testing.T) {
5758

5859
for i, d := range deadlineTests {
5960
t.Run(fmt.Sprintf("testIndex: %v, testDetails: %v", i, d), func(t *testing.T) {
60-
logger := zaptest.NewLogger(t)
61+
logger := testlogger.NewZap(t)
6162
a := &testActivityDeadline{logger: logger}
6263
registry := newRegistry()
6364
registry.addActivityWithLock(a.ActivityType().Name, a)
@@ -101,7 +102,7 @@ func TestActivityTaskHandler_Execute_deadline(t *testing.T) {
101102
}
102103

103104
func TestActivityTaskHandler_Execute_worker_stop(t *testing.T) {
104-
logger := zaptest.NewLogger(t)
105+
logger := testlogger.NewZap(t)
105106

106107
a := &testActivityDeadline{logger: logger}
107108
registry := newRegistry()
@@ -150,7 +151,7 @@ func TestActivityTaskHandler_Execute_worker_stop(t *testing.T) {
150151
}
151152

152153
func TestActivityTaskHandler_Execute_with_propagators(t *testing.T) {
153-
logger := zaptest.NewLogger(t)
154+
logger := testlogger.NewZap(t)
154155

155156
now := time.Now()
156157

@@ -208,7 +209,7 @@ func TestActivityTaskHandler_Execute_with_propagators(t *testing.T) {
208209
}
209210

210211
func TestActivityTaskHandler_Execute_with_propagator_failure(t *testing.T) {
211-
logger := zaptest.NewLogger(t)
212+
logger := testlogger.NewZap(t)
212213

213214
now := time.Now()
214215

@@ -254,7 +255,7 @@ func TestActivityTaskHandler_Execute_with_propagator_failure(t *testing.T) {
254255
}
255256

256257
func TestActivityTaskHandler_Execute_with_auto_heartbeat(t *testing.T) {
257-
logger := zaptest.NewLogger(t)
258+
logger := testlogger.NewZap(t)
258259

259260
now := time.Now()
260261

internal/activity_test.go

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,12 +24,13 @@ import (
2424
"context"
2525
"testing"
2626

27+
"go.uber.org/cadence/internal/common/testlogger"
28+
2729
"github.com/golang/mock/gomock"
2830
"github.com/stretchr/testify/require"
2931
"github.com/stretchr/testify/suite"
3032
"go.uber.org/yarpc"
3133
"go.uber.org/zap"
32-
"go.uber.org/zap/zaptest"
3334

3435
"go.uber.org/cadence/.gen/go/cadence/workflowservicetest"
3536
"go.uber.org/cadence/.gen/go/shared"
@@ -56,7 +57,7 @@ func TestActivityTestSuite(t *testing.T) {
5657
func (s *activityTestSuite) SetupTest() {
5758
s.mockCtrl = gomock.NewController(s.T())
5859
s.service = workflowservicetest.NewMockClient(s.mockCtrl)
59-
s.logger = zaptest.NewLogger(s.T())
60+
s.logger = testlogger.NewZap(s.T())
6061
}
6162

6263
func (s *activityTestSuite) TearDownTest() {

internal/auto_heartbeater_test.go

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -26,10 +26,11 @@ import (
2626
"testing"
2727
"time"
2828

29+
"go.uber.org/cadence/internal/common/testlogger"
30+
2931
"github.com/jonboulle/clockwork"
3032
"github.com/stretchr/testify/assert"
3133
"github.com/stretchr/testify/require"
32-
"go.uber.org/zap/zaptest"
3334

3435
"go.uber.org/cadence/.gen/go/shared"
3536
"go.uber.org/cadence/internal/common"
@@ -51,7 +52,7 @@ func TestAutoHearbeater_Run(t *testing.T) {
5152
t.Run("worker stop channel", func(t *testing.T) {
5253
stopCh := make(chan struct{})
5354
invoker := &MockServiceInvoker{}
54-
logger := zaptest.NewLogger(t)
55+
logger := testlogger.NewZap(t)
5556
clock := clockwork.NewFakeClock()
5657
hearbeater := newHeartbeater(stopCh, invoker, logger, clock, activityType, workflowExecution)
5758

@@ -62,7 +63,7 @@ func TestAutoHearbeater_Run(t *testing.T) {
6263
t.Run("context done", func(t *testing.T) {
6364
stopCh := make(chan struct{})
6465
invoker := &MockServiceInvoker{}
65-
logger := zaptest.NewLogger(t)
66+
logger := testlogger.NewZap(t)
6667
clock := clockwork.NewFakeClock()
6768
hearbeater := newHeartbeater(stopCh, invoker, logger, clock, activityType, workflowExecution)
6869

@@ -75,7 +76,7 @@ func TestAutoHearbeater_Run(t *testing.T) {
7576
stopCh := make(chan struct{})
7677
invoker := &MockServiceInvoker{}
7778
invoker.EXPECT().BackgroundHeartbeat().Return(nil).Once()
78-
logger := zaptest.NewLogger(t)
79+
logger := testlogger.NewZap(t)
7980
clock := clockwork.NewFakeClock()
8081
hearbeater := newHeartbeater(stopCh, invoker, logger, clock, activityType, workflowExecution)
8182

@@ -98,7 +99,7 @@ func TestAutoHearbeater_Run(t *testing.T) {
9899
stopCh := make(chan struct{})
99100
invoker := &MockServiceInvoker{}
100101
invoker.EXPECT().BackgroundHeartbeat().Return(assert.AnError).Once()
101-
logger := zaptest.NewLogger(t)
102+
logger := testlogger.NewZap(t)
102103
clock := clockwork.NewFakeClock()
103104
hearbeater := newHeartbeater(stopCh, invoker, logger, clock, activityType, workflowExecution)
104105

Lines changed: 143 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,143 @@
1+
// Copyright (c) 2017-2021 Uber Technologies Inc.
2+
//
3+
// Permission is hereby granted, free of charge, to any person obtaining a copy
4+
// of this software and associated documentation files (the "Software"), to deal
5+
// in the Software without restriction, including without limitation the rights
6+
// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
7+
// copies of the Software, and to permit persons to whom the Software is
8+
// furnished to do so, subject to the following conditions:
9+
//
10+
// The above copyright notice and this permission notice shall be included in
11+
// all copies or substantial portions of the Software.
12+
//
13+
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
14+
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
15+
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
16+
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
17+
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
18+
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
19+
// THE SOFTWARE.
20+
21+
package testlogger
22+
23+
import (
24+
"fmt"
25+
"slices"
26+
"strings"
27+
28+
"github.com/stretchr/testify/require"
29+
"go.uber.org/atomic"
30+
"go.uber.org/zap"
31+
"go.uber.org/zap/zapcore"
32+
"go.uber.org/zap/zaptest"
33+
"go.uber.org/zap/zaptest/observer"
34+
)
35+
36+
type TestingT interface {
37+
zaptest.TestingT
38+
Cleanup(func()) // not currently part of zaptest.TestingT
39+
}
40+
41+
// NewZap makes a new test-oriented logger that prevents bad-lifecycle logs from failing tests.
42+
func NewZap(t TestingT) *zap.Logger {
43+
/*
44+
HORRIBLE HACK due to async shutdown, both in our code and in libraries:
45+
normally, logs produced after a test finishes will *intentionally* fail the test and/or
46+
cause data to race on the test's internal `t.done` field.
47+
48+
that's a good thing, it reveals possibly-dangerously-flawed lifecycle management.
49+
50+
unfortunately some of our code and some libraries do not have good lifecycle management,
51+
and this cannot easily be patched from the outside.
52+
53+
so this logger cheats: after a test completes, it logs to stderr rather than TestingT.
54+
EVERY ONE of these logs is bad and we should not produce them, but it's causing many
55+
otherwise-useful tests to be flaky, and that's a larger interruption than is useful.
56+
*/
57+
logAfterComplete, err := zap.NewDevelopment()
58+
require.NoError(t, err, "could not build a fallback zap logger")
59+
replaced := &fallbackTestCore{
60+
t: t,
61+
fallback: logAfterComplete.Core(),
62+
testing: zaptest.NewLogger(t).Core(),
63+
completed: &atomic.Bool{},
64+
}
65+
66+
t.Cleanup(replaced.UseFallback) // switch to fallback before ending the test
67+
68+
return zap.New(replaced)
69+
}
70+
71+
// NewObserved makes a new test logger that both logs to `t` and collects logged
72+
// events for asserting in tests.
73+
func NewObserved(t TestingT) (*zap.Logger, *observer.ObservedLogs) {
74+
obsCore, obs := observer.New(zapcore.DebugLevel)
75+
z := NewZap(t)
76+
z = z.WithOptions(zap.WrapCore(func(core zapcore.Core) zapcore.Core {
77+
return zapcore.NewTee(core, obsCore)
78+
}))
79+
return z, obs
80+
}
81+
82+
type fallbackTestCore struct {
83+
t TestingT
84+
fallback zapcore.Core
85+
testing zapcore.Core
86+
completed *atomic.Bool
87+
}
88+
89+
var _ zapcore.Core = (*fallbackTestCore)(nil)
90+
91+
func (f *fallbackTestCore) UseFallback() {
92+
f.completed.Store(true)
93+
}
94+
95+
func (f *fallbackTestCore) Enabled(level zapcore.Level) bool {
96+
if f.completed.Load() {
97+
return f.fallback.Enabled(level)
98+
}
99+
return f.testing.Enabled(level)
100+
}
101+
102+
func (f *fallbackTestCore) With(fields []zapcore.Field) zapcore.Core {
103+
// need to copy and defer, else the returned core will be used at an
104+
// arbitrarily later point in time, possibly after the test has completed.
105+
return &fallbackTestCore{
106+
t: f.t,
107+
fallback: f.fallback.With(fields),
108+
testing: f.testing.With(fields),
109+
completed: f.completed,
110+
}
111+
}
112+
113+
func (f *fallbackTestCore) Check(entry zapcore.Entry, checked *zapcore.CheckedEntry) *zapcore.CheckedEntry {
114+
// see other Check impls, all look similar.
115+
// this defers the "where to log" decision to Write, as `f` is the core that will write.
116+
if f.fallback.Enabled(entry.Level) {
117+
return checked.AddCore(entry, f)
118+
}
119+
return checked // do not add any cores
120+
}
121+
122+
func (f *fallbackTestCore) Write(entry zapcore.Entry, fields []zapcore.Field) error {
123+
if f.completed.Load() {
124+
entry.Message = fmt.Sprintf("COULD FAIL TEST %q, logged too late: %v", f.t.Name(), entry.Message)
125+
126+
hasStack := slices.ContainsFunc(fields, func(field zapcore.Field) bool {
127+
// no specific stack-trace type, so just look for probable fields.
128+
return strings.Contains(strings.ToLower(field.Key), "stack")
129+
})
130+
if !hasStack {
131+
fields = append(fields, zap.Stack("log_stack"))
132+
}
133+
return f.fallback.Write(entry, fields)
134+
}
135+
return f.testing.Write(entry, fields)
136+
}
137+
138+
func (f *fallbackTestCore) Sync() error {
139+
if f.completed.Load() {
140+
return f.fallback.Sync()
141+
}
142+
return f.testing.Sync()
143+
}

0 commit comments

Comments
 (0)