softassert for test hooks

Author: stephanos

common/testing/testhooks/noop_impl.go

@@ -3,24 +3,28 @@
 package testhooks
 
 import (
-	"context"
-
+	"go.temporal.io/server/common/log"
+	"go.temporal.io/server/common/softassert"
 	"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 {
+	logger log.Logger
+}
+
+func NewTestHooks(logger log.Logger) TestHooks {
+	return TestHooks{logger: logger}
+}
 
 // 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.
@@ -31,23 +35,14 @@ func Get[T any](_ TestHooks, key Key) (T, bool) {
 	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) {
-	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) {
 }
 
-// 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](th TestHooks, key Key, val T) func() {
+	softassert.Fail(th.logger, "testhooks.Set called without test_dep build tag")
+	return func() {}
 }

common/testing/testhooks/test_impl.go

@@ -3,14 +3,14 @@
 package testhooks
 
 import (
-	"context"
 	"sync"
 
+	"go.temporal.io/server/common/log"
 	"go.uber.org/fx"
 )
 
 var Module = fx.Options(
-	fx.Provide(NewTestHooksImpl),
+	fx.Provide(NewTestHooks),
 )
 
 type (
@@ -31,10 +31,12 @@ type (
 	testHooksImpl struct {
 		m sync.Map
 	}
-
-	contextKey struct{}
 )
 
+func NewTestHooks(_ log.Logger) TestHooks {
+	return &testHooksImpl{}
+}
+
 // Get gets the value of a test hook from the registry.
 //
 // TestHooks should be used sparingly, see comment on TestHooks.
@@ -50,19 +52,6 @@ func Get[T any](th TestHooks, key Key) (T, bool) {
 	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)
-	}
-	var zero T
-	return zero, false
-}
-
 // Call calls a func() hook if present.
 //
 // TestHooks should be used sparingly, see comment on TestHooks.
@@ -72,49 +61,13 @@ 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)
 }

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(nil)
 
-		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(nil)
+
+	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)
 	})
 }

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(params.Logger),
+		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)