diff --git a/common/testing/testhooks/noop_impl.go b/common/testing/testhooks/noop_impl.go index 44232be0088..a83cbf97ca4 100644 --- a/common/testing/testhooks/noop_impl.go +++ b/common/testing/testhooks/noop_impl.go @@ -3,39 +3,30 @@ package testhooks import ( - "context" - "go.uber.org/fx" ) var Module = fx.Options( - fx.Provide(func() (_ TestHooks) { return }), + fx.Provide(NewTestHooks), ) -type ( - // TestHooks (in production mode) is an empty struct just so the build works. - // See TestHooks in test_impl.go. - // - // TestHooks are an inherently unclean way of writing tests. They require mixing test-only - // concerns into production code. In general you should prefer other ways of writing tests - // wherever possible, and only use TestHooks sparingly, as a last resort. - TestHooks struct{} -) +// TestHooks (in production mode) is an empty struct just so the build works. +// See TestHooks in test_impl.go. +// +// TestHooks are an inherently unclean way of writing tests. They require mixing test-only +// concerns into production code. In general you should prefer other ways of writing tests +// wherever possible, and only use TestHooks sparingly, as a last resort. +type TestHooks struct{} + +func NewTestHooks() TestHooks { + return TestHooks{} +} // Get gets the value of a test hook. In production mode it always returns the zero value and // false, which hopefully the compiler will inline and remove the hook as dead code. // // TestHooks should be used very sparingly, see comment on TestHooks. -func Get[T any](_ TestHooks, key Key) (T, bool) { - var zero T - return zero, false -} - -// GetCtx gets the value of a test hook from the registry embedded in the -// context chain. -// -// TestHooks should be used sparingly, see comment on TestHooks. -func GetCtx[T any](ctx context.Context, key Key) (T, bool) { +func Get[T any](_ TestHooks, _ Key) (T, bool) { var zero T return zero, false } @@ -43,11 +34,10 @@ func GetCtx[T any](ctx context.Context, key Key) (T, bool) { // Call calls a func() hook if present. // // TestHooks should be used very sparingly, see comment on TestHooks. -func Call(_ TestHooks, key Key) { +func Call(_ TestHooks, _ Key) { } -// CallCtx calls a func(context.Context) hook if present. -// -// TestHooks should be used very sparingly, see comment on TestHooks. -func CallCtx(_ context.Context, key Key) { +// Set is only to be used by test code together with the test_dep build tag. +func Set[T any](_ TestHooks, _ Key, _ T) func() { + panic("testhooks.Set called but TestHooks are not enabled: use -tags=test_dep when running `go test`") } diff --git a/common/testing/testhooks/test_impl.go b/common/testing/testhooks/test_impl.go index 8e08977a6b9..f5dac36a2c4 100644 --- a/common/testing/testhooks/test_impl.go +++ b/common/testing/testhooks/test_impl.go @@ -3,63 +3,41 @@ package testhooks import ( - "context" "sync" "go.uber.org/fx" ) var Module = fx.Options( - fx.Provide(NewTestHooksImpl), + fx.Provide(NewTestHooks), ) -type ( - // TestHooks holds a registry of active test hooks. It should be obtained through fx and - // used with Get and Set. - // - // TestHooks are an inherently unclean way of writing tests. They require mixing test-only - // concerns into production code. In general you should prefer other ways of writing tests - // wherever possible, and only use TestHooks sparingly, as a last resort. - TestHooks interface { - // private accessors; access must go through package-level Get/Set - get(Key) (any, bool) - set(Key, any) - del(Key) - } - - // testHooksImpl is an implementation of TestHooks. - testHooksImpl struct { - m sync.Map - } +// TestHooks holds a registry of active test hooks. It should be obtained through fx and +// used with Get and Set. +// +// TestHooks are an inherently unclean way of writing tests. They require mixing test-only +// concerns into production code. In general you should prefer other ways of writing tests +// wherever possible, and only use TestHooks sparingly, as a last resort. +type TestHooks struct { + data *sync.Map +} - contextKey struct{} -) +func NewTestHooks() TestHooks { + return TestHooks{data: &sync.Map{}} +} // Get gets the value of a test hook from the registry. // // TestHooks should be used sparingly, see comment on TestHooks. func Get[T any](th TestHooks, key Key) (T, bool) { var zero T - if th == nil { + if th.data == nil { return zero, false } - if val, ok := th.get(key); ok { + if val, ok := th.data.Load(key); ok { // this is only used in test so we want to panic on type mismatch: - return val.(T), ok // nolint:revive - } - return zero, false -} - -// GetCtx gets the value of a test hook from the registry embedded in the -// context chain. -// -// TestHooks should be used sparingly, see comment on TestHooks. -func GetCtx[T any](ctx context.Context, key Key) (T, bool) { - hooks := ctx.Value(contextKey{}) - if hooks, ok := hooks.(TestHooks); ok { - return Get[T](hooks, key) + return val.(T), ok //nolint:revive } - var zero T return zero, false } @@ -72,57 +50,9 @@ func Call(th TestHooks, key Key) { } } -// CallCtx calls a func() hook if present and a TestHooks implementation -// exists in the context chain. -// -// TestHooks should be used sparingly, see comment on TestHooks. -func CallCtx(ctx context.Context, key Key) { - hooks := ctx.Value(contextKey{}) - if hooks, ok := hooks.(TestHooks); ok { - Call(hooks, key) - } -} - // Set sets a test hook to a value and returns a cleanup function to unset it. // Calls to Set and the cleanup function should form a stack. func Set[T any](th TestHooks, key Key, val T) func() { - th.set(key, val) - return func() { th.del(key) } -} - -// SetCtx sets a test hook to a value on the provided context and returns a -// cleanup function to unset it. Calls to SetCtx and the cleanup function -// should form a stack. -func SetCtx[T any](ctx context.Context, key Key, val T) func() { - hooks := ctx.Value(contextKey{}) - if hooks, ok := hooks.(TestHooks); ok { - return Set(hooks, key, val) - } - return func() {} -} - -// NewTestHooksImpl returns a new instance of a test hook registry. This is provided and used -// in the main "resource" module as a default, but in functional tests, it's overridden by an -// explicitly constructed instance. -func NewTestHooksImpl() TestHooks { - return &testHooksImpl{} -} - -// NewInjectedTestHooksImpl returns a new instance of a test hook registry and a context with the -// registry injected. -func NewInjectedTestHooksImpl(parent context.Context) (context.Context, TestHooks) { - hooks := NewTestHooksImpl() - return context.WithValue(parent, contextKey{}, hooks), hooks -} - -func (th *testHooksImpl) get(key Key) (any, bool) { - return th.m.Load(key) -} - -func (th *testHooksImpl) set(key Key, val any) { - th.m.Store(key, val) -} - -func (th *testHooksImpl) del(key Key) { - th.m.Delete(key) + th.data.Store(key, val) + return func() { th.data.Delete(key) } } diff --git a/common/testing/testhooks/test_impl_test.go b/common/testing/testhooks/test_impl_test.go index c8c1c0cc0bf..f6d17309f9b 100644 --- a/common/testing/testhooks/test_impl_test.go +++ b/common/testing/testhooks/test_impl_test.go @@ -2,51 +2,51 @@ package testhooks -import "testing" - -// Ensure that testhook functionality that operates on contexts functions as -// expected. -func TestTestHooks_Context(t *testing.T) { - t.Run("Values can be set and get on the context directly", func(t *testing.T) { - ctx, _ := NewInjectedTestHooksImpl(t.Context()) - cleanup := SetCtx(ctx, UpdateWithStartInBetweenLockAndStart, 33) - defer cleanup() +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestGet(t *testing.T) { + th := NewTestHooks() - v, ok := GetCtx[int](ctx, UpdateWithStartInBetweenLockAndStart) - if !ok { - t.Fatal("Expected TestHooksImpl to contain value") - } - if v != 33 { - t.Fatal("Expected value to be 33") - } + t.Run("returns false for unset key", func(t *testing.T) { + _, ok := Get[int](th, MatchingDisableSyncMatch) + require.False(t, ok) }) - t.Run("Values set directly on the registry are visible through the context", func(t *testing.T) { - ctx, reg := NewInjectedTestHooksImpl(t.Context()) - cleanup := Set(reg, UpdateWithStartInBetweenLockAndStart, 44) + t.Run("returns set value", func(t *testing.T) { + cleanup := Set(th, MatchingDisableSyncMatch, 42) defer cleanup() - v, ok := GetCtx[int](ctx, UpdateWithStartInBetweenLockAndStart) - if !ok { - t.Fatal("Expected TestHooksImpl to contain value") - } - if v != 44 { - t.Fatal("Expected value to be 44") - } + v, ok := Get[int](th, MatchingDisableSyncMatch) + require.True(t, ok) + require.Equal(t, 42, v) + }) + + t.Run("cleanup removes value", func(t *testing.T) { + cleanup := Set(th, MatchingDisableSyncMatch, 42) + cleanup() + + _, ok := Get[int](th, MatchingDisableSyncMatch) + require.False(t, ok) + }) +} + +func TestCall(t *testing.T) { + th := NewTestHooks() + + t.Run("does nothing for unset key", func(t *testing.T) { + Call(th, MatchingDisableSyncMatch) // should not panic }) - t.Run("CallCtx uses the registry injected into the context", func(t *testing.T) { - ctx, reg := NewInjectedTestHooksImpl(t.Context()) - var value int - callback := func() { - value = 55 - } - cleanup := Set(reg, UpdateWithStartInBetweenLockAndStart, callback) + t.Run("calls set function", func(t *testing.T) { + called := false + cleanup := Set(th, MatchingDisableSyncMatch, func() { called = true }) defer cleanup() - CallCtx(ctx, UpdateWithStartInBetweenLockAndStart) - if value != 55 { - t.Fatal("Expected value to be 55") - } + Call(th, MatchingDisableSyncMatch) + require.True(t, called) }) } diff --git a/docs/development/testing.md b/docs/development/testing.md index fe317bcf7c3..72b3d05780f 100644 --- a/docs/development/testing.md +++ b/docs/development/testing.md @@ -5,7 +5,7 @@ This document describes the project's testing setup, utilities and best practice ## Setup ### Build tags -- `test_dep` (required): This Go build tag is required for running functional tests. +- `test_dep`: This Go build tag enables the test hooks implementation. Only very few tests require it; they will fail if not enabled. - `TEMPORAL_DEBUG`: Extends functional test timeouts to allow sufficient time for debugging sessions. - `disable_grpc_modules`: Disables gRPC modules for faster compilation during unit tests. @@ -136,4 +136,4 @@ See [tracing.md](../../docs/development/tracing.md) for more details. You'll find the code coverage reporting in Codecov: https://app.codecov.io/gh/temporalio/temporal. Consider installing the [Codecov Browser Extension](https://docs.codecov.com/docs/the-codecov-browser-extension) -to see code coverage directly in GitHub PRs. \ No newline at end of file +to see code coverage directly in GitHub PRs. diff --git a/tests/testcore/onebox.go b/tests/testcore/onebox.go index 37708008043..880bee570f5 100644 --- a/tests/testcore/onebox.go +++ b/tests/testcore/onebox.go @@ -214,14 +214,13 @@ func newTemporal(t *testing.T, params *TemporalParams) *TemporalImpl { tlsConfigProvider: params.TLSConfigProvider, captureMetricsHandler: params.CaptureMetricsHandler, dcClient: dynamicconfig.NewMemoryClient(), - // If this doesn't build, make sure you're building with tags 'test_dep': - testHooks: testhooks.NewTestHooksImpl(), - serviceFxOptions: params.ServiceFxOptions, - taskCategoryRegistry: params.TaskCategoryRegistry, - hostsByProtocolByService: params.HostsByProtocolByService, - grpcClientInterceptor: grpcinject.NewInterceptor(), - replicationStreamRecorder: NewReplicationStreamRecorder(), - spanExporters: params.SpanExporters, + testHooks: testhooks.NewTestHooks(), + serviceFxOptions: params.ServiceFxOptions, + taskCategoryRegistry: params.TaskCategoryRegistry, + hostsByProtocolByService: params.HostsByProtocolByService, + grpcClientInterceptor: grpcinject.NewInterceptor(), + replicationStreamRecorder: NewReplicationStreamRecorder(), + spanExporters: params.SpanExporters, } // Configure output file path for on-demand logging (call WriteToLog() to write)