From 35af54d6aa1b27d2b643ee157aebd654c3e1c02a Mon Sep 17 00:00:00 2001 From: Ethan <39577870+ethanndickson@users.noreply.github.com> Date: Thu, 18 Jun 2026 00:21:09 +1000 Subject: [PATCH] test: isolate passive chatd internal tests (#26369) Make `newInternalTestServer` use option functions for logger, clock, and worker startup, and make it passive by default so internal chatd tests only opt into background execution when they need a real worker. Use the passive server path in `TestAwaitSubagentCompletion` for the state-driven subtests, keep `ContextCanceled` explicitly active for real provider cancellation coverage, and keep the fail-fast default AI provider base URL so accidental provider calls still fail immediately. Closes CODAGT-586 Closes https://github.com/coder/internal/issues/1549 --- coderd/database/dbgen/dbgen.go | 13 ++- coderd/x/chatd/recording_internal_test.go | 2 +- coderd/x/chatd/subagent_internal_test.go | 135 +++++++++------------- 3 files changed, 65 insertions(+), 85 deletions(-) diff --git a/coderd/database/dbgen/dbgen.go b/coderd/database/dbgen/dbgen.go index b56ce64ee0..3ccad638aa 100644 --- a/coderd/database/dbgen/dbgen.go +++ b/coderd/database/dbgen/dbgen.go @@ -218,12 +218,13 @@ func AIProvider(t testing.TB, db database.Store, seed database.AIProvider, munge displayName = sql.NullString{String: name, Valid: true} } params := database.InsertAIProviderParams{ - ID: id, - Type: provType, - Name: name, - DisplayName: displayName, - Enabled: takeFirst(seed.Enabled, true), - BaseUrl: takeFirst(seed.BaseUrl, "https://api.example.com/"), + ID: id, + Type: provType, + Name: name, + DisplayName: displayName, + Enabled: takeFirst(seed.Enabled, true), + // Use an unsupported scheme so leaked test provider calls fail immediately without retries. + BaseUrl: takeFirst(seed.BaseUrl, "invalid://test.invalid/"), Settings: seed.Settings, SettingsKeyID: seed.SettingsKeyID, } diff --git a/coderd/x/chatd/recording_internal_test.go b/coderd/x/chatd/recording_internal_test.go index 0ad40a97b9..f4b576a1ea 100644 --- a/coderd/x/chatd/recording_internal_test.go +++ b/coderd/x/chatd/recording_internal_test.go @@ -520,7 +520,7 @@ func TestWaitAgentTimeoutLeavesRecordingRunning(t *testing.T) { mockConn := agentconnmock.NewMockAgentConn(ctrl) // Use the mock clock server; don't set agentConnFn yet. - server := newInternalTestServerWithClock(t, db, ps, chatprovider.ProviderAPIKeys{}, mClock) + server := newInternalTestServer(t, db, ps, chatprovider.ProviderAPIKeys{}, withInternalTestServerClock(mClock)) user, org, model := seedInternalChatDeps(t, db) workspace, _, agent := seedWorkspaceBinding(t, db, user.ID) diff --git a/coderd/x/chatd/subagent_internal_test.go b/coderd/x/chatd/subagent_internal_test.go index 176c0d5d57..4f324380f8 100644 --- a/coderd/x/chatd/subagent_internal_test.go +++ b/coderd/x/chatd/subagent_internal_test.go @@ -69,73 +69,64 @@ func TestSubagentFallbackChatTitle(t *testing.T) { } } -// newInternalTestServer creates a Server for internal tests with -// custom provider API keys. The server is automatically closed -// when the test finishes. +type internalTestServerConfig struct { + logger slog.Logger + clock quartz.Clock + startWorker bool +} + +type internalTestServerOpt func(*internalTestServerConfig) + +func withInternalTestServerClock(clk quartz.Clock) internalTestServerOpt { + return func(cfg *internalTestServerConfig) { + cfg.clock = clk + } +} + +func withInternalTestServerLogger(logger slog.Logger) internalTestServerOpt { + return func(cfg *internalTestServerConfig) { + cfg.logger = logger + } +} + +func withInternalTestServerWorker() internalTestServerOpt { + return func(cfg *internalTestServerConfig) { + cfg.startWorker = true + } +} + +// newInternalTestServer creates a passive Server for internal tests with +// custom provider API keys. Pass withInternalTestServerWorker to start the +// background chat worker for tests that need real execution. func newInternalTestServer( t *testing.T, db database.Store, ps pubsub.Pubsub, keys chatprovider.ProviderAPIKeys, -) *Server { - return newInternalTestServerWithLoggerAndClock( - t, - db, - ps, - keys, - slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}), - nil, - ) -} - -func newInternalTestServerWithClock( - t *testing.T, - db database.Store, - ps pubsub.Pubsub, - keys chatprovider.ProviderAPIKeys, - clk quartz.Clock, -) *Server { - return newInternalTestServerWithLoggerAndClock( - t, - db, - ps, - keys, - slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}), - clk, - ) -} - -func newInternalTestServerWithLogger( - t *testing.T, - db database.Store, - ps pubsub.Pubsub, - keys chatprovider.ProviderAPIKeys, - logger slog.Logger, -) *Server { - return newInternalTestServerWithLoggerAndClock(t, db, ps, keys, logger, nil) -} - -func newInternalTestServerWithLoggerAndClock( - t *testing.T, - db database.Store, - ps pubsub.Pubsub, - keys chatprovider.ProviderAPIKeys, - logger slog.Logger, - clk quartz.Clock, + opts ...internalTestServerOpt, ) *Server { t.Helper() + cfg := internalTestServerConfig{ + logger: slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}), + } + for _, opt := range opts { + opt(&cfg) + } + server := New(ps, Config{ - Logger: logger, + Logger: cfg.logger, Database: db, ReplicaID: uuid.New(), - Clock: clk, + Clock: cfg.clock, // Use a very long interval so the background loop // does not interfere with test assertions. PendingChatAcquireInterval: testutil.WaitLong, ProviderAPIKeys: keys, }) - server.Start() + if cfg.startWorker { + server.Start() + } t.Cleanup(func() { require.NoError(t, server.Close()) }) @@ -491,7 +482,8 @@ func TestResolveUserProviderAPIKeys_AIProvider(t *testing.T) { keys, err := server.resolveUserProviderAPIKeys(ctx, user.ID, provider.ID) require.NoError(t, err) require.Equal(t, "user-api-key", keys.APIKey("openai")) - require.Equal(t, "https://api.example.com/", keys.BaseURL("openai")) + // The expected URL is dbgen's default AIProvider BaseUrl. + require.Equal(t, "invalid://test.invalid/", keys.BaseURL("openai")) }) t.Run("ProviderKeyUsedWhenBYOKDisabled", func(t *testing.T) { @@ -1305,7 +1297,7 @@ func TestSpawnAgent_GeneralOverrideLogsAndFallsBackWhenCredentialsUnavailable(t db, ps := dbtestutil.NewDB(t) logSink := &subagentTestLogSink{} logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).AppendSinks(logSink) - server := newInternalTestServerWithLogger(t, db, ps, chatprovider.ProviderAPIKeys{}, logger) + server := newInternalTestServer(t, db, ps, chatprovider.ProviderAPIKeys{}, withInternalTestServerLogger(logger)) ctx := chatdTestContext(t) user, org, model := seedInternalChatDeps(t, db) @@ -1364,7 +1356,7 @@ func TestSpawnAgent_GeneralOverrideLogsAndFallsBackWhenProviderDisabled(t *testi db, ps := dbtestutil.NewDB(t) logSink := &subagentTestLogSink{} logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).AppendSinks(logSink) - server := newInternalTestServerWithLogger( + server := newInternalTestServer( t, db, ps, @@ -1373,7 +1365,7 @@ func TestSpawnAgent_GeneralOverrideLogsAndFallsBackWhenProviderDisabled(t *testi "openai-compat": "fallback-key", }, }, - logger, + withInternalTestServerLogger(logger), ) ctx := chatdTestContext(t) @@ -2446,7 +2438,7 @@ func TestSpawnAgent_ComputerUseRejectsInvalidConfiguredProviderWithStableReason( require.NoError(t, db.UpsertChatComputerUseProvider(ctx, "bogus")) logSink := &subagentTestLogSink{} logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).AppendSinks(logSink) - server := newInternalTestServerWithLogger(t, db, ps, chatprovider.ProviderAPIKeys{}, logger) + server := newInternalTestServer(t, db, ps, chatprovider.ProviderAPIKeys{}, withInternalTestServerLogger(logger)) user, org, model := seedInternalChatDeps(t, db) parentChat := createInternalParentChat( @@ -3370,8 +3362,7 @@ func TestAwaitSubagentCompletion(t *testing.T) { // Shared fixtures for subtests that use a real clock. Each // subtest creates its own parent+child chats (unique IDs) // so they don't collide. Mock-clock subtests need their own - // DB and server because the Server's background tickers - // also use the mock clock. + // DB and server so the wait loop's timers stay isolated. db, ps := dbtestutil.NewDB(t) server := newInternalTestServer(t, db, ps, chatprovider.ProviderAPIKeys{}) user, org, model := seedInternalChatDeps(t, db) @@ -3455,7 +3446,7 @@ func TestAwaitSubagentCompletion(t *testing.T) { db, _ := dbtestutil.NewDB(t) mClock := quartz.NewMock(t) ps := subscribeFailingPubsub{Pubsub: pubsub.NewInMemory()} - server := newInternalTestServerWithClock(t, db, ps, chatprovider.ProviderAPIKeys{}, mClock) + server := newInternalTestServer(t, db, ps, chatprovider.ProviderAPIKeys{}, withInternalTestServerClock(mClock)) ctx := chatdTestContext(t) user, org, model := seedInternalChatDeps(t, db) @@ -3504,15 +3495,12 @@ func TestAwaitSubagentCompletion(t *testing.T) { db, ps := dbtestutil.NewDB(t) mClock := quartz.NewMock(t) - server := newInternalTestServerWithClock(t, db, ps, chatprovider.ProviderAPIKeys{}, mClock) + server := newInternalTestServer(t, db, ps, chatprovider.ProviderAPIKeys{}, withInternalTestServerClock(mClock)) ctx := chatdTestContext(t) user, org, model := seedInternalChatDeps(t, db) parent, child := createParentChildChats(ctx, t, server, user, org, model) - // signalWake from CreateChat may trigger immediate processing. - // Wait for it to settle, then reset chats to the state we need. - server.drainInflight() setChatStatus(ctx, t, db, parent.ID, database.ChatStatusRunning, "") setChatStatus(ctx, t, db, child.ID, database.ChatStatusRunning, "") @@ -3559,15 +3547,9 @@ func TestAwaitSubagentCompletion(t *testing.T) { require.NoError(t, err) defer cancelProbe() - // Insert the message BEFORE transitioning to Waiting. - // Stale PG LISTEN/NOTIFY notifications from the - // processor's earlier run can still be buffered in the - // pgListener after drainInflight returns. If such a - // notification is dispatched between setChatStatus and - // insertAssistantMessage, checkSubagentCompletion would - // see done=true (Waiting) with an empty report. By - // inserting the message first, the report is guaranteed - // to be committed before the status makes it visible. + // Insert the message before transitioning to Waiting so any + // notification observing the terminal status can also read the + // committed report. insertAssistantMessage(t, db, child.ID, model.ID, "pubsub result") setChatStatus(ctx, t, db, child.ID, database.ChatStatusWaiting, "") require.EventuallyWithT(t, func(c *assert.CollectT) { @@ -3595,11 +3577,8 @@ func TestAwaitSubagentCompletion(t *testing.T) { parent, child := createParentChildChats(ctx, t, server, user, org, model) - // signalWake from CreateChat may trigger immediate processing. - // Wait for it to settle, then set the terminal state we need. // This case should return immediately, so use the shared - // real-clock server instead of a mock clock. - server.drainInflight() + // real-clock passive server instead of a mock clock. setChatStatus(ctx, t, db, child.ID, database.ChatStatusWaiting, "") gotChat, report, err := server.awaitSubagentCompletion( @@ -3615,7 +3594,7 @@ func TestAwaitSubagentCompletion(t *testing.T) { db, ps := dbtestutil.NewDB(t) mClock := quartz.NewMock(t) - server := newInternalTestServerWithClock(t, db, ps, chatprovider.ProviderAPIKeys{}, mClock) + server := newInternalTestServer(t, db, ps, chatprovider.ProviderAPIKeys{}, withInternalTestServerClock(mClock)) ctx := chatdTestContext(t) user, org, model := seedInternalChatDeps(t, db) @@ -3671,7 +3650,7 @@ func TestAwaitSubagentCompletion(t *testing.T) { }) db, ps := dbtestutil.NewDB(t) - server := newInternalTestServer(t, db, ps, chatprovider.ProviderAPIKeys{}) + server := newInternalTestServer(t, db, ps, chatprovider.ProviderAPIKeys{}, withInternalTestServerWorker()) ctx := chatdTestContext(t) user, org, _ := seedInternalChatDeps(t, db) provider := dbgen.ChatProvider(t, db, database.ChatProvider{