panic test hooks if not enabled

common/testing/testhooks/noop_impl.go

@@ -3,51 +3,38 @@
 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{}
-)
+// noopTestHooks is a noop implementation of TestHooks for production builds.
+type noopTestHooks struct{}
+
+func NewTestHooks() TestHooks {
+	return noopTestHooks{}
+}
+
+func (noopTestHooks) 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
 }
 
 // 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`")
 }

common/testing/testhooks/test_impl.go

@@ -3,37 +3,25 @@
 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.
+type testHooksImpl struct {
+	m sync.Map
+}
 
-	// testHooksImpl is an implementation of TestHooks.
-	testHooksImpl struct {
-		m sync.Map
-	}
+func NewTestHooks() TestHooks {
+	return &testHooksImpl{}
+}
 
-	contextKey struct{}
-)
+func (*testHooksImpl) testHooks() {}
 
 // Get gets the value of a test hook from the registry.
 //
@@ -43,23 +31,14 @@ func Get[T any](th TestHooks, key Key) (T, bool) {
 	if th == nil {
 		return zero, false
 	}
-	if val, ok := th.get(key); ok {
-		// this is only used in test so we want to panic on type mismatch:
-		return val.(T), ok // nolint:revive
+	impl, ok := th.(*testHooksImpl)
+	if !ok {
+		return zero, false
 	}
-	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)
+	if val, ok := impl.m.Load(key); ok {
+		// this is only used in test so we want to panic on type mismatch:
+		return val.(T), ok //nolint:revive
 	}
-	var zero T
 	return zero, false
 }
 
@@ -72,57 +51,10 @@ 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)
+	impl := th.(*testHooksImpl)
+	impl.m.Store(key, val)
+	return func() { impl.m.Delete(key) }
 }

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)
 	})
 }

common/testing/testhooks/testhooks.go

@@ -0,0 +1,11 @@
+package testhooks
+
+// TestHooks holds a registry of active test hooks. It should be obtained through fx and
+// used with Get and Call.
+//
+// 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 interface {
+	testHooks()
+}

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.
+to see code coverage directly in GitHub PRs.

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)