Skip to content

Commit b51e891

Browse files
authored
[common] Introduce testlogger as a workaround of poor lifecycle (#1398)
* [common] Introduce testlogger as a workaround for a poor lifecycle
1 parent 5ec4386 commit b51e891

20 files changed

+427
-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

internal/common/convert.go

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,3 +95,17 @@ func QueryResultTypePtr(t s.QueryResultType) *s.QueryResultType {
9595
func PtrOf[T any](v T) *T {
9696
return &v
9797
}
98+
99+
// ValueFromPtr returns the value from a pointer.
100+
func ValueFromPtr[T any](v *T) T {
101+
if v == nil {
102+
return Zero[T]()
103+
}
104+
return *v
105+
}
106+
107+
// Zero returns the zero value of a type by return type.
108+
func Zero[T any]() T {
109+
var zero T
110+
return zero
111+
}

internal/common/convert_test.go

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,3 +55,20 @@ func TestCeilHelpers(t *testing.T) {
5555
assert.Equal(t, int32(2), Int32Ceil(1.1))
5656
assert.Equal(t, int64(2), Int64Ceil(1.1))
5757
}
58+
59+
func TestValueFromPtr(t *testing.T) {
60+
assert.Equal(t, "a", ValueFromPtr(PtrOf("a")))
61+
assert.Equal(t, 1, ValueFromPtr(PtrOf(1)))
62+
assert.Equal(t, int32(1), ValueFromPtr(PtrOf(int32(1))))
63+
assert.Equal(t, int64(1), ValueFromPtr(PtrOf(int64(1))))
64+
assert.Equal(t, 1.1, ValueFromPtr(PtrOf(1.1)))
65+
assert.Equal(t, true, ValueFromPtr(PtrOf(true)))
66+
assert.Equal(t, []string{"a"}, ValueFromPtr(PtrOf([]string{"a"})))
67+
assert.Equal(t, "" /* default value */, ValueFromPtr((*string)(nil)))
68+
}
69+
70+
func TestZero(t *testing.T) {
71+
assert.Equal(t, "", Zero[string]())
72+
assert.Equal(t, 0, Zero[int]())
73+
assert.Equal(t, (*int)(nil), Zero[*int]())
74+
}
Lines changed: 163 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,163 @@
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+
"sync"
28+
29+
"go.uber.org/cadence/internal/common"
30+
31+
"github.com/stretchr/testify/require"
32+
"go.uber.org/zap"
33+
"go.uber.org/zap/zapcore"
34+
"go.uber.org/zap/zaptest"
35+
"go.uber.org/zap/zaptest/observer"
36+
)
37+
38+
type TestingT interface {
39+
zaptest.TestingT
40+
Cleanup(func()) // not currently part of zaptest.TestingT
41+
}
42+
43+
// NewZap makes a new test-oriented logger that prevents bad-lifecycle logs from failing tests.
44+
func NewZap(t TestingT) *zap.Logger {
45+
/*
46+
HORRIBLE HACK due to async shutdown, both in our code and in libraries:
47+
normally, logs produced after a test finishes will *intentionally* fail the test and/or
48+
cause data to race on the test's internal `t.done` field.
49+
50+
that's a good thing, it reveals possibly-dangerously-flawed lifecycle management.
51+
52+
unfortunately some of our code and some libraries do not have good lifecycle management,
53+
and this cannot easily be patched from the outside.
54+
55+
so this logger cheats: after a test completes, it logs to stderr rather than TestingT.
56+
EVERY ONE of these logs is bad and we should not produce them, but it's causing many
57+
otherwise-useful tests to be flaky, and that's a larger interruption than is useful.
58+
*/
59+
logAfterComplete, err := zap.NewDevelopment()
60+
require.NoError(t, err, "could not build a fallback zap logger")
61+
replaced := &fallbackTestCore{
62+
mu: &sync.RWMutex{},
63+
t: t,
64+
fallback: logAfterComplete.Core(),
65+
testing: zaptest.NewLogger(t).Core(),
66+
completed: common.PtrOf(false),
67+
}
68+
69+
t.Cleanup(replaced.UseFallback) // switch to fallback before ending the test
70+
71+
return zap.New(replaced)
72+
}
73+
74+
// NewObserved makes a new test logger that both logs to `t` and collects logged
75+
// events for asserting in tests.
76+
func NewObserved(t TestingT) (*zap.Logger, *observer.ObservedLogs) {
77+
obsCore, obs := observer.New(zapcore.DebugLevel)
78+
z := NewZap(t)
79+
z = z.WithOptions(zap.WrapCore(func(core zapcore.Core) zapcore.Core {
80+
return zapcore.NewTee(core, obsCore)
81+
}))
82+
return z, obs
83+
}
84+
85+
type fallbackTestCore struct {
86+
mu *sync.RWMutex
87+
t TestingT
88+
fallback zapcore.Core
89+
testing zapcore.Core
90+
completed *bool
91+
}
92+
93+
var _ zapcore.Core = (*fallbackTestCore)(nil)
94+
95+
func (f *fallbackTestCore) UseFallback() {
96+
f.mu.Lock()
97+
defer f.mu.Unlock()
98+
*f.completed = true
99+
}
100+
101+
func (f *fallbackTestCore) Enabled(level zapcore.Level) bool {
102+
f.mu.RLock()
103+
defer f.mu.RUnlock()
104+
if f.completed != nil && *f.completed {
105+
return f.fallback.Enabled(level)
106+
}
107+
return f.testing.Enabled(level)
108+
}
109+
110+
func (f *fallbackTestCore) With(fields []zapcore.Field) zapcore.Core {
111+
f.mu.Lock()
112+
defer f.mu.Unlock()
113+
114+
// need to copy and defer, else the returned core will be used at an
115+
// arbitrarily later point in time, possibly after the test has completed.
116+
return &fallbackTestCore{
117+
mu: f.mu,
118+
t: f.t,
119+
fallback: f.fallback.With(fields),
120+
testing: f.testing.With(fields),
121+
completed: f.completed,
122+
}
123+
}
124+
125+
func (f *fallbackTestCore) Check(entry zapcore.Entry, checked *zapcore.CheckedEntry) *zapcore.CheckedEntry {
126+
f.mu.RLock()
127+
defer f.mu.RUnlock()
128+
// see other Check impls, all look similar.
129+
// this defers the "where to log" decision to Write, as `f` is the core that will write.
130+
if f.fallback.Enabled(entry.Level) {
131+
return checked.AddCore(entry, f)
132+
}
133+
return checked // do not add any cores
134+
}
135+
136+
func (f *fallbackTestCore) Write(entry zapcore.Entry, fields []zapcore.Field) error {
137+
f.mu.RLock()
138+
defer f.mu.RUnlock()
139+
140+
if common.ValueFromPtr(f.completed) {
141+
entry.Message = fmt.Sprintf("COULD FAIL TEST %q, logged too late: %v", f.t.Name(), entry.Message)
142+
143+
hasStack := slices.ContainsFunc(fields, func(field zapcore.Field) bool {
144+
// no specific stack-trace type, so just look for probable fields.
145+
return strings.Contains(strings.ToLower(field.Key), "stack")
146+
})
147+
if !hasStack {
148+
fields = append(fields, zap.Stack("log_stack"))
149+
}
150+
return f.fallback.Write(entry, fields)
151+
}
152+
return f.testing.Write(entry, fields)
153+
}
154+
155+
func (f *fallbackTestCore) Sync() error {
156+
f.mu.RLock()
157+
defer f.mu.RUnlock()
158+
159+
if common.ValueFromPtr(f.completed) {
160+
return f.fallback.Sync()
161+
}
162+
return f.testing.Sync()
163+
}

0 commit comments

Comments
 (0)