From e59c94949c2b5a66a45bc8cac0bcfdb5c05776a8 Mon Sep 17 00:00:00 2001 From: "Matthew M. Keeler" Date: Fri, 15 May 2026 16:57:41 -0400 Subject: [PATCH] feat: Assert hook execution order for evaluation and track stages (#341) --- mockld/hook_callback_service.go | 10 +++ sdktests/common_tests_hooks.go | 126 ++++++++++++++++++++++++++++++++ sdktests/testapi_hooks.go | 17 ++++- servicedef/command_params.go | 5 ++ 4 files changed, 156 insertions(+), 2 deletions(-) diff --git a/mockld/hook_callback_service.go b/mockld/hook_callback_service.go index d7328ead..2e6d5cfa 100644 --- a/mockld/hook_callback_service.go +++ b/mockld/hook_callback_service.go @@ -4,6 +4,7 @@ import ( "encoding/json" "io" "net/http" + "sync/atomic" "github.com/launchdarkly/sdk-test-harness/v2/framework" "github.com/launchdarkly/sdk-test-harness/v2/framework/harness" @@ -24,9 +25,14 @@ func (h *HookCallbackService) Close() { h.payloadEndpoint.Close() } +// NewHookCallbackService creates an HTTP endpoint that records incoming hook +// callbacks. If sequence is non-nil it is incremented per received call and +// stamped onto the payload so tests can establish ordering across hooks that +// share the same counter. func NewHookCallbackService( testHarness *harness.TestHarness, logger framework.Logger, + sequence *atomic.Int64, ) *HookCallbackService { h := &HookCallbackService{ CallChannel: make(chan servicedef.HookExecutionPayload), @@ -49,6 +55,10 @@ func NewHookCallbackService( return } + if sequence != nil { + response.Sequence = sequence.Add(1) + } + go func() { h.CallChannel <- response }() diff --git a/sdktests/common_tests_hooks.go b/sdktests/common_tests_hooks.go index 7bc809eb..4aa5f433 100644 --- a/sdktests/common_tests_hooks.go +++ b/sdktests/common_tests_hooks.go @@ -1,6 +1,8 @@ package sdktests import ( + "cmp" + "slices" "strconv" "github.com/launchdarkly/go-sdk-common/v3/ldcontext" @@ -29,6 +31,10 @@ func doEvaluationSeriesTests(t *ldtest.T) { t.Run("executes beforeEvaluation stage", executesBeforeEvaluationStage) t.Run("executes afterEvaluation stage", executesAfterEvaluationStage) t.Run("an error in before stage does not affect after stage", errorInBeforeStageDoesNotAffectAfterStage) + t.Run("executes beforeEvaluation hooks in registration order", + executesBeforeEvaluationHooksInRegistrationOrder) + t.Run("executes afterEvaluation hooks in reverse registration order", + executesAfterEvaluationHooksInReverseRegistrationOrder) t.Run("data propagates from before to after", beforeEvaluationDataPropagatesToAfter) t.RequireCapability(servicedef.CapabilityMigrations) @@ -39,6 +45,7 @@ func doTrackSeriesTests(t *ldtest.T) { t.RequireCapability(servicedef.CapabilityTrackHooks) t.Run("executes afterTrack stage", executesAfterTrackStage) t.Run("a hook error prevents afterTrack stage", errorInHookPreventsAfterTrackStage) + t.Run("executes afterTrack hooks in registration order", executesAfterTrackHooksInRegistrationOrder) } func executesBeforeEvaluationStage(t *ldtest.T) { @@ -472,6 +479,125 @@ func errorInHookPreventsAfterTrackStage(t *ldtest.T) { hooks.ExpectNoCall(t, hookName) } +// hookOrderTestNames returns numHooks distinct hook names suitable for +// ordering tests. The names embed their registration index so the order in +// which the SDK executed them is decoded from the names alone. +func hookOrderTestNames(prefix string, numHooks int) []string { + names := make([]string, 0, numHooks) + for i := 0; i < numHooks; i++ { + names = append(names, prefix+"-"+strconv.Itoa(i)) + } + return names +} + +// observedHookOrder collects one call per hook at the given stage and returns +// the hook names sorted by harness-stamped sequence number, i.e. the order +// the SDK actually executed them. +func observedHookOrder(t *ldtest.T, hooks *Hooks, names []string, stage servicedef.HookStage) []string { + type observedCall struct { + name string + sequence int64 + } + calls := make([]observedCall, 0, len(names)) + for _, name := range names { + hooks.ExpectCall(t, name, func(payload servicedef.HookExecutionPayload) bool { + if payload.Stage.Value() != stage { + return false + } + calls = append(calls, observedCall{name: name, sequence: payload.Sequence}) + return true + }) + } + slices.SortFunc(calls, func(a, b observedCall) int { return cmp.Compare(a.sequence, b.sequence) }) + out := make([]string, 0, len(calls)) + for _, c := range calls { + out = append(out, c.name) + } + return out +} + +// afterTrack must execute in the order of hook registration (forward), +// unlike afterEvaluation/afterIdentify which run in reverse-registration order. +func executesAfterTrackHooksInRegistrationOrder(t *ldtest.T) { + names := hookOrderTestNames("afterTrackOrderHook", 3) + + context := ldcontext.New("user-key") + eventContext := o.Some(context) + configurers := []SDKConfigurer{} + + if t.Capabilities().Has(servicedef.CapabilityClientSide) { + configurers = append(configurers, WithClientSideInitialContext(context)) + eventContext = o.None[ldcontext.Context]() + } + + client, hooks := createClientForHooks(t, names, nil, configurers...) + defer hooks.Close() + + client.SendCustomEvent(t, servicedef.CustomEventParams{ + EventKey: "custom-event", + Context: eventContext, + }) + + assert.Equal(t, names, observedHookOrder(t, hooks, names, servicedef.AfterTrack), + "afterTrack hooks must execute in the order of hook registration") +} + +// beforeEvaluation must execute in the order of hook registration. +func executesBeforeEvaluationHooksInRegistrationOrder(t *ldtest.T) { + names := hookOrderTestNames("beforeEvalOrderHook", 3) + + context := ldcontext.New("user-key") + flagContext := o.Some(context) + configurers := []SDKConfigurer{} + + if t.Capabilities().Has(servicedef.CapabilityClientSide) { + configurers = append(configurers, WithClientSideInitialContext(context)) + flagContext = o.None[ldcontext.Context]() + } + + client, hooks := createClientForHooks(t, names, nil, configurers...) + defer hooks.Close() + + client.EvaluateFlag(t, servicedef.EvaluateFlagParams{ + FlagKey: "bool-flag", + Context: flagContext, + ValueType: servicedef.ValueTypeBool, + DefaultValue: ldvalue.Bool(false), + }) + + assert.Equal(t, names, observedHookOrder(t, hooks, names, servicedef.BeforeEvaluation), + "beforeEvaluation hooks must execute in the order of hook registration") +} + +// afterEvaluation must execute in the reverse of the order of hook registration. +func executesAfterEvaluationHooksInReverseRegistrationOrder(t *ldtest.T) { + names := hookOrderTestNames("afterEvalOrderHook", 3) + + context := ldcontext.New("user-key") + flagContext := o.Some(context) + configurers := []SDKConfigurer{} + + if t.Capabilities().Has(servicedef.CapabilityClientSide) { + configurers = append(configurers, WithClientSideInitialContext(context)) + flagContext = o.None[ldcontext.Context]() + } + + client, hooks := createClientForHooks(t, names, nil, configurers...) + defer hooks.Close() + + client.EvaluateFlag(t, servicedef.EvaluateFlagParams{ + FlagKey: "bool-flag", + Context: flagContext, + ValueType: servicedef.ValueTypeBool, + DefaultValue: ldvalue.Bool(false), + }) + + expected := slices.Clone(names) + slices.Reverse(expected) + assert.Equal(t, expected, observedHookOrder(t, hooks, names, servicedef.AfterEvaluation), + "afterEvaluation hooks must execute in the reverse of the order of hook registration") +} + func createClientForHooks(t *ldtest.T, instances []string, hookData map[servicedef.HookStage]servicedef.SDKConfigEvaluationHookData, configurers ...SDKConfigurer) (*SDKClient, *Hooks) { diff --git a/sdktests/testapi_hooks.go b/sdktests/testapi_hooks.go index 1b65142b..086b1c0a 100644 --- a/sdktests/testapi_hooks.go +++ b/sdktests/testapi_hooks.go @@ -1,6 +1,7 @@ package sdktests import ( + "sync/atomic" "time" "github.com/stretchr/testify/assert" @@ -26,6 +27,14 @@ type HookInstance struct { type Hooks struct { instances map[string]HookInstance + // order preserves the registration order of hook names so Configure can + // send hooks to the SDK deterministically. Map iteration in Go is + // randomized, so we cannot derive this from `instances` alone. + order []string + // sequence is a per-Hooks counter shared by every HookCallbackService so + // that arrival order across hooks (which each have their own callback URL) + // can be reconstructed in tests. + sequence *atomic.Int64 } func NewHooks( @@ -37,14 +46,17 @@ func NewHooks( ) *Hooks { hooks := &Hooks{ instances: make(map[string]HookInstance), + order: make([]string, 0, len(instances)), + sequence: &atomic.Int64{}, } for _, instance := range instances { hooks.instances[instance] = HookInstance{ name: instance, - hookService: mockld.NewHookCallbackService(testHarness, logger), + hookService: mockld.NewHookCallbackService(testHarness, logger, hooks.sequence), data: data, errors: errors, } + hooks.order = append(hooks.order, instance) } return hooks @@ -52,7 +64,8 @@ func NewHooks( func (h *Hooks) Configure(config *servicedef.SDKConfigParams) error { hookConfig := config.Hooks.Value() - for _, instance := range h.instances { + for _, name := range h.order { + instance := h.instances[name] hookConfig.Hooks = append(hookConfig.Hooks, servicedef.SDKConfigHookInstance{ Name: instance.name, CallbackURI: instance.hookService.GetURL(), diff --git a/servicedef/command_params.go b/servicedef/command_params.go index 1747f566..2eb1ed13 100644 --- a/servicedef/command_params.go +++ b/servicedef/command_params.go @@ -224,6 +224,11 @@ type HookExecutionPayload struct { EvaluationDetail o.Maybe[EvaluateFlagResponse] `json:"evaluationDetail"` TrackSeriesContext o.Maybe[TrackSeriesContext] `json:"trackSeriesContext"` Stage o.Maybe[HookStage] `json:"stage"` + + // Sequence is stamped by the test harness in the order callbacks arrive. + // Shared across all hook instances of a single Hooks group so tests can + // assert cross-hook ordering. Not part of the SDK contract. + Sequence int64 `json:"-"` } // RegisterFlagChangeListenerParams defines parameters for registering a general flag change listener.