refactor: create internal/integrationtest package infrastructure by pawbana · Pull Request #206 · coder/aibridge

pawbana · 2026-03-06T13:01:04Z

Refactor integration tests into internal/integrationtest package

This PR consolidates all integration tests into a dedicated internal/integrationtest package.

Move test infrastructure to internal/integrationtest

Move upstream.go and mockmcp.go from internal/testutil to internal/integrationtest
Create setupbridge.go with newBridgeTestServer and functional options pattern
Create helpers.go with shared configuration builders and utilities

Migrate all integration tests

Move integration test files from root to internal/integrationtest/:

bridge_integration_test.go → bridge_test.go
metrics_integration_test.go → metrics_test.go
trace_integration_test.go → trace_test.go
responses_integration_test.go → responses_test.go
circuit_breaker_integration_test.go → circuit_breaker_test.go
apidump_integration_test.go → apidump_test.go

Replace boilerplate with functional options

Convert all aibridge.NewRequestBridge call sites to use newBridgeTestServer with options:

Replace inline bridge setup patterns with withProvider(), withMetrics(), withMCP(), etc.
Eliminate configureFunc closures and repetitive server setup code
Standardize actor context setup with withActor()

Consolidate test utilities

Remove duplicate helper functions across test files
Standardize request creation with bridgeTestServer.makeRequest()
Use consistent constants (apiKey, defaultActorID, path constants)
Centralize provider configuration builders

pawbana · 2026-03-06T13:01:19Z

This stack of pull requests is managed by Graphite. Learn more about stacking.

dannykopping

Love the more concise tests! Thanks for doing this.

I would've appreciated if you'd stacked these changes. It's a huge PR with not only deduplication but also logical changes, and this makes it very difficult to review effectively. I'm concerned that some things will slip through, but in general it looks good.

dannykopping · 2026-03-09T15:31:13Z

internal/integrationtest/setupbridge.go

+	for _, h := range header {
+		for k, vals := range h {
+			for _, v := range vals {
+				req.Header.Set(k, v)


This should be Header.Add right? Otherwise the last one always wins?

dannykopping · 2026-03-09T15:32:28Z

internal/integrationtest/bridge_test.go

 			require.Len(t, received, 1)
 			require.Equal(t, tc.expectedUpstreamPath, received[0].Path)
-			require.Contains(t, received[0].Header.Get(provider.AuthHeader()), apiKey)
+			require.Contains(t, received[0].Header.Get(tc.authHeader), apiKey)


You're hardcoding the auth header now instead of using provider.AuthHeader; why?

After refactor provider variable is no longer here.
This change is done because of it but in general hardcoding test inputs / expected outputs, in my experience was desired.

I'd prefer we didn't define this upstream requirement in multiple places.

But, I can see why you did it and change it in the provider will cause the tests to fail, so it's not a big deal.

ssncferreira

Overall LGTM, this makes the tests cleaner 🧹 thanks for working on this!
Agree with Danny that in the future it would be nice to split big PRs like this into smaller, more focused ones to make the review process easier 🙂

ssncferreira · 2026-03-10T09:31:56Z

internal/integrationtest/bridge_test.go

-		configureFunc  func(*testing.T, string, aibridge.Recorder) (*aibridge.RequestBridge, error)
-		createRequest  func(t *testing.T, baseURL string, body []byte) *http.Request
+		header         http.Header
+		mutateBody     func(t *testing.T, body []byte) []byte


Nit: IIUC mutateBody is only used once to set metadata.user_id, maybe a simple string field like sessionUserID would be clearer? We can always promote to a func later if needed.

ssncferreira · 2026-03-10T09:38:15Z

internal/integrationtest/bridge_test.go

+				resp := bridgeServer.makeRequest(t, http.MethodPost, tc.path, reqBody)
+				// Drain the body so streaming responses complete without
+				// a "connection reset" error in the mock upstream.
+				_, _ = io.ReadAll(resp.Body)


We should still check for errors here, right? 🤔

Suggested change

_, _ = io.ReadAll(resp.Body)

_, err := io.ReadAll(resp.Body)

require.NoError(t, err)

ssncferreira · 2026-03-10T09:52:05Z

internal/integrationtest/bridge_test.go

-			provider.NewAnthropic(anthropicCfg("http://unused", apiKey), bedrockCfg),
-		}, recorderClient, testutil.NewNoopMCPManager(), logger, nil, testTracer)
-		require.NoError(t, err)
+		bridgeServer := newBridgeTestServer(t, ctx, "http://unused",


newBridgeTestServer calls newLogger(t) which does not set IgnoreErrors: true as we had before, isn't this breaking the tests? 🤔

Test pass, I'm not sure what changed that there are no errors now.

ssncferreira · 2026-03-10T09:53:20Z

internal/integrationtest/responses_test.go

-			srv, _ := newTestSrv(t, ctx, prov, nil, testTracer)
-			defer srv.Close()
+			resp := bridgeServer.makeRequest(t, http.MethodPost, pathOpenAIResponses, []byte(tc.request))
+			_, _ = io.ReadAll(resp.Body)


We should still check the error:

Suggested change

_, _ = io.ReadAll(resp.Body)

_, err = io.ReadAll(resp.Body)

require.NoError(t, err)

ssncferreira · 2026-03-10T09:58:52Z

internal/integrationtest/setupbridge.go

+// withCustomProvider adds a pre-built provider. The upstream URL passed to
+// [newBridgeTestServer] is ignored for this provider.
+// When any provider option is used, the default "all providers" set is not created.
+func withCustomProvider(p aibridge.Provider) bridgeOption {


Nit: what happens when we set both withProvider and withCustomProvider? This means we could be appending 2 anthropic providers, right?

It is possible to append 2 same providers just using withProvider.
With 2 the same providers test will fail due to panic, http mux will panic due to not being able to mount routes with the same path. For testing utility I think it is fine not to check for it.

panic: pattern "/openai/v1/chat/completions" (registered at /home/coder/aibridge/bridge.go:106) conflicts with pattern "/openai/v1/chat/completions" (registered at /home/coder/aibridge/bridge.go:106):

ssncferreira · 2026-03-10T10:07:29Z

internal/integrationtest/setupbridge.go

+	resp := bridgeServer.makeRequest(t, http.MethodPost, path, reqBody)
+	require.Equal(t, http.StatusOK, resp.StatusCode)
+
+	// We must ALWAYS have 2 calls to the bridge for injected tool tests.


IIUC, the 2 upstream calls are:

The initial request, where the provider responds with "call tool X"

The follow-up request, where aibridge sends the tool result back to the provider

Is this correct? Could we make the comment a bit clearer about this?

ssncferreira · 2026-03-10T10:11:57Z

internal/integrationtest/setupbridge.go

+		return upstream.Calls.Load() == 2
+	}, time.Second*10, time.Millisecond*50)
+
+	return bridgeServer.Recorder, mockMCP, resp


Nit: setupInjectedToolTest creates a bridgeTestServer but only returns the Recorder. Would it make sense to return *bridgeTestServer instead? This way callers can still access .Recorder, but also have access to the test server if needed.

- Move upstream.go and mockmcp.go from internal/testutil to internal/integrationtest - Create bridge.go with NewBridgeTestServer, NewLogger, DefaultActorID, DefaultTracer - Create requests.go with shared request/config helpers (promoted from test files) - internal/testutil retains only lightweight mocks (MockRecorder, MockProvider)

…egrationtest/ - Move metrics_integration_test.go → internal/integrationtest/metrics_test.go - Move trace_integration_test.go → internal/integrationtest/trace_test.go - Change package from aibridge_test to integrationtest_test - Replace all newTestSrv calls with integrationtest.NewBridgeTestServer - Delete the newTestSrv helper function entirely - Update all imports: testutil symbols → integrationtest where moved - Replace local helpers (apiKey, userID, openaiCfg, etc.) with exported versions - All former newTestSrv sites use WithWrappedRecorder() to preserve behavior - Remove duplicate testBedrockCfg (already in bridge_test.go)

… tests to internal/integrationtest/ Move the following files from the repo root: - responses_integration_test.go → internal/integrationtest/responses_test.go - circuit_breaker_integration_test.go → internal/integrationtest/circuit_breaker_test.go - apidump_integration_test.go → internal/integrationtest/apidump_test.go Changes in all files: - Package: aibridge_test → integrationtest_test - Replace newTestSrv/NewRequestBridge with integrationtest.NewBridgeTestServer - Replace testutil.* moved symbols with integrationtest.* - Replace local helper refs (openaiCfg, apiKey, userID, etc.) with integrationtest.OpenAICfg, integrationtest.APIKey, integrationtest.DefaultActorID - Delete local createOpenAIResponsesReq (now in requests.go) - Keep local helpers specific to each file

…lerplate with NewBridgeTestServer - Convert all 18 remaining aibridge.NewRequestBridge call sites to use NewBridgeTestServer with functional options - Replace all configureFunc closures (4 different signatures, ~12 inline closures) with providerFn field + NewBridgeTestServer in test body - Convert setupInjectedToolTest to use NewBridgeTestServer internally - Fix stale recorderClient references to ts.Recorder - Remove unused imports (slog, aibcontext, testutil from responses_test) - Delete bridge_integration_test.go from root Net result: -424 lines of boilerplate, root directory down to 6 files. All tests pass (go test ./... -count=1).

Change package from integrationtest_test to integrationtest, remove the self-import, and strip all integrationtest. prefixes from identifiers.

Switch all _test.go files from package integrationtest_test to package integrationtest. Since nothing outside this directory imports the package, all exported symbols are unexported: - Types: BridgeTestServer → bridgeTestServer, MockMCP → mockMCP, MockUpstream → mockUpstream, UpstreamResponse → upstreamResponse, etc. - Functions: NewBridgeTestServer → newBridgeTestServer, CreateAnthropicMessagesReq → createAnthropicMessagesReq, etc. - Options: WithMetrics → withMetrics, WithTracer → withTracer, etc. - Constants: APIKey → apiKey, DefaultActorID → defaultActorID, etc. - Methods: GetCallsByTool → getCallsByTool, ReceivedRequests → receivedRequests, etc. This eliminates the integrationtest. prefix from all call sites and removes the artificial export surface. All tests pass (go test ./... -count=1).

…p.Response return type makeRequest now performs the HTTP request internally and returns *http.Response directly. Remove the intermediate http.Client creation, client.Do(req), and require.NoError(t, err) calls at all 5 call sites.

makeRequest now returns *http.Response directly instead of *http.Request. Updated all 6 call sites: - Removed intermediate http.DefaultClient.Do / client.Do calls - Removed require.NoError for the HTTP request error - Collapsed header mutation site to pass http.Header as extra arg

makeRequest now returns *http.Response directly (performs HTTP request internally). Updated all 14 call sites: - Changed req := to resp := - Removed http.DefaultClient.Do(req) and require.NoError(t, err) lines - Removed client := &http.Client{} and client.Do(req) patterns - Converted header mutation case to pass http.Header as extra arg

…etions to use newBridgeTestServer + test some names cleanup

…derName from TestFallthrough

pawbana · 2026-03-10T12:16:19Z

Merge activity

Mar 10, 12:16 PM UTC: A user started a stack merge that includes this pull request via Graphite.
Mar 10, 12:16 PM UTC: @pawbana merged this pull request with Graphite.

pawbana force-pushed the pb/testing-cleanup-new-request-bridge branch from f45d4d2 to e95970f Compare March 6, 2026 13:04

pawbana marked this pull request as ready for review March 6, 2026 15:15

pawbana requested review from dannykopping and ssncferreira March 6, 2026 15:41

pawbana force-pushed the pb/testing-cleanup-new-request-bridge branch from fd1cc42 to e235208 Compare March 6, 2026 16:42

dannykopping approved these changes Mar 9, 2026

View reviewed changes

pawbana mentioned this pull request Mar 9, 2026

feat: add client label to InterceptionCount, PromptCount and TokenUseCount metrics #209

Merged

ssncferreira approved these changes Mar 10, 2026

View reviewed changes

pawbana added 21 commits March 10, 2026 11:36

refactor: convert bridge_test.go from external to internal test package

d6e6df0

Change package from integrationtest_test to integrationtest, remove the self-import, and strip all integrationtest. prefixes from identifiers.

refactor: split bridge_test.go into per-interceptor test files

23eb779

fixup: apply unexport renames to remaining helper files

11f6f37

remove providerFunc

82ea368

merge tests back into bridge_test.go

e3d0f14

bridge.go -> setupbridge.go + helpers.go

741133c

remove withWrappedRecorder

c0541df

remove actorID arg from setupInjectedToolTest

e7aaf92

use bridgeTestServer.newRequest

94e7ccc

bridgeServer.makeRequest + tracerSetup + TestSessionIDTracking cleanup

9755905

cleanup

b48a153

reorder tests in bridge_test.go to original order

8860414

remove not needed withLogger

89a70b7

pawbana added 11 commits March 10, 2026 11:36

fix comments

cfa535f

rename

5bf7bc7

formatting fix

dfa21f0

reorder to previous

a482ec1

recorder cleanup

4291a3a

MockRecorder Total*Tokens

b0e32be

add Body.Close() to bridgeServer.makeRequest + some test names cleanup

8f49730

changed TestResponsesParallelToolsOverwritten and TestOpenAIChatCompl…

cf4208e

…etions to use newBridgeTestServer + test some names cleanup

fmt fix

1086b3b

review 1: fixed setting headers in makeRequest, removed unneded provi…

41e9598

…derName from TestFallthrough

review 2

0f25518

pawbana force-pushed the pb/testing-cleanup-new-request-bridge branch from b24c6fc to 0f25518 Compare March 10, 2026 11:37

pawbana merged commit b01456d into main Mar 10, 2026
5 checks passed

	_, _ = io.ReadAll(resp.Body)
	_, err := io.ReadAll(resp.Body)
	require.NoError(t, err)

	_, _ = io.ReadAll(resp.Body)
	_, err = io.ReadAll(resp.Body)
	require.NoError(t, err)

Conversation

pawbana commented Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Refactor integration tests into internal/integrationtest package

Move test infrastructure to internal/integrationtest

Migrate all integration tests

Replace boilerplate with functional options

Consolidate test utilities

Uh oh!

pawbana commented Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dannykopping left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ssncferreira left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ssncferreira Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pawbana commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge activity

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pawbana commented Mar 6, 2026 •

edited

Loading

pawbana commented Mar 6, 2026 •

edited

Loading

ssncferreira Mar 10, 2026 •

edited

Loading

pawbana commented Mar 10, 2026 •

edited

Loading