From 71c815e9161df0fb360edf920550e31b41975355 Mon Sep 17 00:00:00 2001 From: Jae Gangemi Date: Sun, 12 Jul 2026 15:13:24 -0600 Subject: [PATCH] feat(testkit): tool-list-changed assertion helper (MC-47) - wire mcpx.ClientOptions.OnToolListChanged into testkit.Harness at New(), signaling a buffered non-blocking channel on each notification - add Harness.WaitForToolListChanged(timeout) bool: waits for at least one notifications/tools/list_changed within timeout, false on timeout - add AssertToolListChanged(t, h, timeout) test-failing wrapper, matching AssertToolSet/EventuallyContains' t-taking style - refactor MC-45's TestLockBeforeConnectThenUnlockAtRuntime to use testkit.Harness + the new helper instead of a hand-built raw mcpx.Client - add TestHarnessToolListChanged_Timeout and _Signaled covering both WaitForToolListChanged branches directly in the testkit package Co-Authored-By: Claude Opus 4.8 --- lock_unlock_test.go | 42 +++++------------------------ testkit/assert.go | 11 ++++++++ testkit/harness.go | 44 ++++++++++++++++++++++++++++-- testkit/harness_internal_test.go | 46 ++++++++++++++++++++++++++++++++ testkit/harness_test.go | 42 +++++++++++++++++++++++++++++ 5 files changed, 147 insertions(+), 38 deletions(-) create mode 100644 testkit/harness_internal_test.go diff --git a/lock_unlock_test.go b/lock_unlock_test.go index cfcefae..f44ab1a 100644 --- a/lock_unlock_test.go +++ b/lock_unlock_test.go @@ -32,53 +32,23 @@ func (hwGroupCap) Attach(r *mcpkit.Registrar) error { // TestLockBeforeConnectThenUnlockAtRuntime proves the lazy-tier mechanism: // locking a group before the app ever connects keeps that group's tools out // of the very first tools/list, and a runtime Unlock later brings them in -// and notifies the connected client via -// notifications/tools/list_changed (observed here through mcpx.Client's -// OnToolListChanged, since that requires a raw mcpx client rather than -// testkit's harness). +// and notifies the connected client via notifications/tools/list_changed +// (observed here via testkit.AssertToolListChanged, MC-47). func TestLockBeforeConnectThenUnlockAtRuntime(t *testing.T) { app, err := mcpkit.New(mcpkit.Info{Name: "lazy-tier", Version: "0.0.1"}, generic.New(), hwGroupCap{}) require.NoError(t, err) require.NoError(t, app.Lock("hw")) - ctx := context.Background() - serverT, clientT := mcpx.InMemoryPair() - - srvSess, err := app.Connect(ctx, serverT) - require.NoError(t, err) - t.Cleanup(func() { _ = srvSess.Close() }) - - changed := make(chan struct{}, 4) - client := mcpx.NewClient(mcpx.Implementation{Name: "lazy-tier-client", Version: "0.0.1"}, &mcpx.ClientOptions{ - OnToolListChanged: func(_ context.Context) { - changed <- struct{}{} - }, - }) - clientSess, err := client.Connect(ctx, clientT) - require.NoError(t, err) - t.Cleanup(func() { _ = clientSess.Close() }) + h := testkit.New(t, app) - tools, err := clientSess.ListTools(ctx) - require.NoError(t, err) - require.Len(t, tools.Tools, 1, "locked group's tool must not appear in the initial tools/list") - require.Equal(t, "ungrouped-tool", tools.Tools[0].Name) + testkit.AssertToolSet(t, h, "ungrouped-tool") require.NoError(t, app.Unlock("hw")) - select { - case <-changed: - case <-time.After(5 * time.Second): - t.Fatal("did not receive tool list changed notification after Unlock") - } + testkit.AssertToolListChanged(t, h, 5*time.Second) - tools, err = clientSess.ListTools(ctx) - require.NoError(t, err) - names := make([]string, 0, len(tools.Tools)) - for _, tool := range tools.Tools { - names = append(names, tool.Name) - } - require.ElementsMatch(t, []string{"hw-tool", "ungrouped-tool"}, names, "Unlock must bring the group's tool into tools/list") + testkit.AssertToolSet(t, h, "hw-tool", "ungrouped-tool") } // TestLockAtRuntimeUnregistersTool proves a runtime Lock (called after the diff --git a/testkit/assert.go b/testkit/assert.go index 6a5b195..491cf6e 100644 --- a/testkit/assert.go +++ b/testkit/assert.go @@ -38,6 +38,17 @@ func EventuallyContains(t testing.TB, timeout, interval time.Duration, fn func() }, timeout, interval, "expected %q to eventually appear", want) } +// AssertToolListChanged fails the test unless the harness observes at least +// one notifications/tools/list_changed notification within timeout. The +// go-sdk debounces rapid successive list changes into a single +// notification, so this asserts "at least one arrived," never an exact +// count. +func AssertToolListChanged(t testing.TB, h *Harness, timeout time.Duration) { + t.Helper() + require.True(t, h.WaitForToolListChanged(timeout), + "expected a tools/list_changed notification within %s", timeout) +} + // AssertToolSet asserts that the app's advertised tools/list is exactly // want, guarding against silent drift. func AssertToolSet(t testing.TB, h *Harness, want ...string) { diff --git a/testkit/harness.go b/testkit/harness.go index 04784bf..b2bdfd2 100644 --- a/testkit/harness.go +++ b/testkit/harness.go @@ -7,6 +7,7 @@ import ( "context" "sync" "testing" + "time" "github.com/dangernoodle-io/mcpkit" "github.com/dangernoodle-io/mcpkit/mcpx" @@ -28,6 +29,15 @@ type Harness struct { mu sync.Mutex progress map[any][]ProgressEvent + + // toolListChanged is signaled (non-blocking) each time the client + // receives a notifications/tools/list_changed notification. Buffered so + // the client's receive goroutine never blocks on a slow/absent waiter; + // callers only need to observe that at least one arrived within a + // timeout, not an exact count (the go-sdk debounces bursts ~10ms apart + // into a single notification, so an exact-count assertion would be + // flaky by design). + toolListChanged chan struct{} } // New composes app over an in-memory transport pair, connects a client, and @@ -46,10 +56,15 @@ func New(t testing.TB, app *mcpkit.App) *Harness { _ = srvSess.Close() }) - h := &Harness{t: t, progress: make(map[any][]ProgressEvent)} + h := &Harness{ + t: t, + progress: make(map[any][]ProgressEvent), + toolListChanged: make(chan struct{}, 8), + } client := mcpx.NewClient(mcpx.Implementation{Name: "testkit", Version: "0.0.0"}, &mcpx.ClientOptions{ - OnProgress: h.recordProgress, + OnProgress: h.recordProgress, + OnToolListChanged: h.recordToolListChanged, }) sess, err := client.Connect(ctx, clientT) @@ -74,6 +89,31 @@ func (h *Harness) recordProgress(_ context.Context, token any, message string, p }) } +// recordToolListChanged runs on the client's receive goroutine; it must +// never block, so the signal is a non-blocking send into a buffered +// channel. +func (h *Harness) recordToolListChanged(_ context.Context) { + select { + case h.toolListChanged <- struct{}{}: + default: + } +} + +// WaitForToolListChanged blocks until the harness observes at least one +// notifications/tools/list_changed notification, or timeout elapses. It +// returns true on the former, false on the latter. Because the go-sdk +// debounces bursts of list changes into a single notification (~10ms), +// callers should not assert an exact count — one call observes "at least +// one arrived." +func (h *Harness) WaitForToolListChanged(timeout time.Duration) bool { + select { + case <-h.toolListChanged: + return true + case <-time.After(timeout): + return false + } +} + // CallTool calls the named tool with args, which must be JSON-marshalable. func (h *Harness) CallTool(ctx context.Context, name string, args any) (*mcpx.CallToolResult, error) { return h.session.CallTool(ctx, name, args) diff --git a/testkit/harness_internal_test.go b/testkit/harness_internal_test.go new file mode 100644 index 0000000..ba78c43 --- /dev/null +++ b/testkit/harness_internal_test.go @@ -0,0 +1,46 @@ +package testkit + +import ( + "context" + "sync" + "testing" +) + +// TestRecordToolListChanged_BufferFull proves that recordToolListChanged +// handles a full buffer correctly by dropping signals in the default case +// (non-blocking send). This tests the drop branch at line ~98 of harness.go. +// +// The test constructs a Harness with the buffered channel, calls +// recordToolListChanged bufferSize+1 times in rapid succession (no draining), +// and asserts it neither blocks nor panics. The first bufferSize sends succeed +// (channel fills), and the (+1)th send hits the non-blocking default clause, +// proving the drop branch is reachable and safe. +func TestRecordToolListChanged_BufferFull(t *testing.T) { + const bufferSize = 8 + + // Construct a minimal Harness with the toolListChanged channel at the + // same buffer size as the real constructor uses (line 62 of harness.go). + h := &Harness{ + t: t, + mu: sync.Mutex{}, + progress: make(map[any][]ProgressEvent), + toolListChanged: make(chan struct{}, bufferSize), + } + + // Send bufferSize + 1 signals. The first bufferSize hit the + // successful send case (case h.toolListChanged <- struct{}{}:). + // The (+1)th hits the default: case, exercising the drop branch. + // + // If recordToolListChanged blocks on any send, this test will hang + // (test timeout). If it panics, the panic surfaces immediately. + for i := 0; i < bufferSize+1; i++ { + h.recordToolListChanged(context.Background()) + } + + // Verify the channel is exactly full (bufferSize sends succeeded, + // the (+1)th was dropped). + if len(h.toolListChanged) != bufferSize { + t.Fatalf("expected channel to contain %d signals (all successful sends before drop), got %d", + bufferSize, len(h.toolListChanged)) + } +} diff --git a/testkit/harness_test.go b/testkit/harness_test.go index 653a6b6..5732a7d 100644 --- a/testkit/harness_test.go +++ b/testkit/harness_test.go @@ -97,3 +97,45 @@ func TestHarnessProgress(t *testing.T) { // A different token must not see this call's events. require.Empty(t, h.ProgressEvents("unrelated-token")) } + +// TestHarnessToolListChanged_Timeout proves WaitForToolListChanged returns +// false when no notifications/tools/list_changed notification arrives +// within the timeout (nothing in this test triggers one). +func TestHarnessToolListChanged_Timeout(t *testing.T) { + app, err := mcpkit.New(mcpkit.Info{Name: "no-change-test", Version: "0.0.1"}, generic.New(), pingCap{}) + require.NoError(t, err) + + h := testkit.New(t, app) + + require.False(t, h.WaitForToolListChanged(20*time.Millisecond)) +} + +type lockedToolCap struct{} + +func (lockedToolCap) Attach(r *mcpkit.Registrar) error { + mcpkit.AddTool(r, &mcpx.Tool{ + Name: "locked-tool", + Description: "d", + }, mcpkit.ReadOnly, func(_ context.Context, _ *mcpx.CallToolRequest, _ struct{}) (*mcpx.CallToolResult, pingOut, error) { + return nil, pingOut{}, nil + }, mcpkit.Group("locked")) + return nil +} + +// TestHarnessToolListChanged_Signaled proves WaitForToolListChanged (and its +// AssertToolListChanged wrapper) observe a real +// notifications/tools/list_changed notification fired by a runtime Unlock. +func TestHarnessToolListChanged_Signaled(t *testing.T) { + app, err := mcpkit.New(mcpkit.Info{Name: "list-changed-test", Version: "0.0.1"}, generic.New(), lockedToolCap{}) + require.NoError(t, err) + + require.NoError(t, app.Lock("locked")) + + h := testkit.New(t, app) + testkit.AssertToolSet(t, h) + + require.NoError(t, app.Unlock("locked")) + + testkit.AssertToolListChanged(t, h, 5*time.Second) + testkit.AssertToolSet(t, h, "locked-tool") +}