test: Add additional MCP transport context integration tests #529
Conversation
- Add integration tests for transport context propagation between MCP clients and servers - Test both sync and async server implementations across all transport types (stateless, streamable, SSE) - Cover Spring WebFlux and WebMVC environments with dedicated test suites - Validate context flow through HTTP headers for authentication, correlation IDs, and metadata - Rename existing McpTransportContextIntegrationTests to SyncServerMcpTransportContextIntegrationTests for clarity Signed-off-by: Christian Tzolov <christian.tzolov@broadcom.com>
There was a problem hiding this comment.
Nice and thorough work! We have a much better confidence in transports with this. Thanks.
I wonder if we don't have a structural issue with these tests, even though that existed before.
We always build all server transports as the field of classes. And then each test only calls .close() on the server that it is using, which in turns closes the underlying server transport - but other transports in the class never have their .close() called. Is that a problem, or will everything be magically garbage-collected away? Maybe @chemicL has insights for us.
(Also, if a test fails, mcpServer.close() is never called).
| private static final ThreadLocal<String> CLIENT_SIDE_HEADER_VALUE_HOLDER = new ThreadLocal<>(); | ||
|
|
||
| private static final String HEADER_NAME = "x-test"; | ||
|
|
||
| private final Supplier<McpTransportContext> clientContextProvider = () -> { | ||
| var headerValue = CLIENT_SIDE_HEADER_VALUE_HOLDER.get(); | ||
| return headerValue != null ? McpTransportContext.create(Map.of("client-side-header-value", headerValue)) | ||
| : McpTransportContext.EMPTY; | ||
| }; |
There was a problem hiding this comment.
[suggestion] I don't think we need to test sync clients and async clients. Sync clients are already tested with Sync servers ; hopefully all servers and all clients are spec-compliant, and breaking something in a client should result in an issue, now matter what type of server is used (and vice-versa). Otherwise the matrix starts to get quite big.
So I suggest:
- SyncServer tests -> use sync clients
- AsyncServer test -> use async clients
This way they are "naturally" paired - even though sync server + async client and async server + sync client would also work.
| private final McpSyncHttpClientRequestCustomizer clientRequestCustomizer = (builder, method, endpoint, body, | ||
| context) -> { | ||
| var headerValue = context.get("client-side-header-value"); | ||
| if (headerValue != null) { | ||
| builder.header(HEADER_NAME, headerValue.toString()); | ||
| } | ||
| }; |
There was a problem hiding this comment.
[suggestion] Let's use an async request customizer. It should change anything but serves as an example.
| @Configuration | ||
| static class TestCommonConfig { | ||
|
|
||
| @Bean | ||
| public McpTransportContextExtractor<ServerRequest> serverContextExtractor() { | ||
| return (ServerRequest r) -> { | ||
| String headerValue = r.servletRequest().getHeader(HEADER_NAME); | ||
| return headerValue != null ? McpTransportContext.create(Map.of("server-side-header-value", headerValue)) | ||
| : McpTransportContext.EMPTY; | ||
| }; | ||
| }; | ||
|
|
||
| } |
There was a problem hiding this comment.
[question] does this need to be a bean wrapped in a config class? It holds a static function that could be used "as-is".
| private final McpSyncClient streamableClient = McpClient | ||
| .sync(HttpClientStreamableHttpTransport.builder("http://localhost:" + PORT) | ||
| .httpRequestCustomizer(clientRequestCustomizer) | ||
| .build()) | ||
| .transportContextProvider(clientContextProvider) | ||
| .build(); |
There was a problem hiding this comment.
[suggestion] swap the order, the beans are between fields of this class.
| var mcpServer = McpServer.sync(statelessServerTransport) | ||
| .capabilities(McpSchema.ServerCapabilities.builder().tools(true).build()) | ||
| .tools(new McpStatelessServerFeatures.SyncToolSpecification(tool, statelessHandler)) | ||
| .build(); |
There was a problem hiding this comment.
[suggestion] Make the server a bean. This way, all the servery bits are in a config class.
|
|
||
| // Async client context provider | ||
| ExchangeFilterFunction asyncClientContextProvider = (request, next) -> Mono.deferContextual(ctx -> { | ||
| var context = ctx.getOrDefault(McpTransportContext.KEY, McpTransportContext.EMPTY); |
There was a problem hiding this comment.
[nit] clarify distinction between reactor context (ctx) and McpTransportContext
| var context = ctx.getOrDefault(McpTransportContext.KEY, McpTransportContext.EMPTY); | |
| var transportContext = ctx.getOrDefault(McpTransportContext.KEY, McpTransportContext.EMPTY); |
| private static final ThreadLocal<String> SYNC_CLIENT_SIDE_HEADER_VALUE_HOLDER = new ThreadLocal<>(); | ||
|
|
||
| private static final String HEADER_NAME = "x-test"; | ||
|
|
||
| // Sync client context provider | ||
| private final Supplier<McpTransportContext> syncClientContextProvider = () -> { | ||
| var headerValue = SYNC_CLIENT_SIDE_HEADER_VALUE_HOLDER.get(); | ||
| return headerValue != null ? McpTransportContext.create(Map.of("client-side-header-value", headerValue)) | ||
| : McpTransportContext.EMPTY; | ||
| }; |
There was a problem hiding this comment.
Same comment as in AsyncServerMcpTransportContextIntegrationTests : consider using only async clients here.
…ove resource cleanup - Remove synchronous client tests from AsyncServerMcpTransportContextIntegrationTests - Clean up unused sync client imports and ThreadLocal context providers - Add proper resource cleanup for async clients in teardown methods - Update documentation to reflect async-only client/server focus - Refactor WebMVC tests to use Spring beans for MCP server configuration - Simplify test architecture by separating sync and async concerns Signed-off-by: Christian Tzolov <christian.tzolov@broadcom.com>
|
@Kehrlann , thanks for the review! |
|
Looks good. Thanks for the updates! |
This change adds missing test coverage for the MCP transport context feature, which allows passing contextual information (like authentication tokens, correlation IDs, or custom metadata) between MCP clients and servers through HTTP headers. The tests ensure that context propagation works correctly across:
Breaking Changes
None. This PR only adds new test files and renames one existing test file for clarity. No production code or APIs are changed.
Types of changes
Checklist