From b8cb19f481e82fe24d847a02fda4a25cf79d173e Mon Sep 17 00:00:00 2001 From: Daniel Garnier-Moiroux Date: Mon, 2 Nov 2026 19:14:10 +0100 Subject: [PATCH] Servlet tests: use one Tomcat per test Revert the core of the change of #1118, and use one Tomcat per test instead of per class. McpSyncListFilteringIntegrationTests, which shared a Tomcat and a transport per server type, now gets a fresh one for each test too. With a shared Tomcat, a stream still being written to after its client went away can corrupt a connection of a later test, which then fails with "HTTP/1.1 header parser received no bytes". Bind test Tomcats to 127.0.0.1 in TomcatTestUtil, rather than in HttpServletSseIntegrationTests only: with the wildcard address, every stop waits a full second on macOS. This makes the per-test classes as fast as, or faster than, they were with a shared Tomcat. Signed-off-by: Daniel Garnier-Moiroux --- .../HttpServletSseIntegrationTests.java | 56 ++++++++---------- .../HttpServletStatelessIntegrationTests.java | 16 +++--- ...ervletStatelessSharedIntegrationTests.java | 51 +++++++---------- ...HttpServletStreamableIntegrationTests.java | 57 ++++++++----------- .../server/McpCompletionTests.java | 16 +++--- .../McpSyncListFilteringIntegrationTests.java | 42 +++++++------- .../server/transport/TomcatTestUtil.java | 8 ++- 7 files changed, 109 insertions(+), 137 deletions(-) diff --git a/mcp-test/src/test/java/io/modelcontextprotocol/server/HttpServletSseIntegrationTests.java b/mcp-test/src/test/java/io/modelcontextprotocol/server/HttpServletSseIntegrationTests.java index a12f6c8da..9777cfe7b 100644 --- a/mcp-test/src/test/java/io/modelcontextprotocol/server/HttpServletSseIntegrationTests.java +++ b/mcp-test/src/test/java/io/modelcontextprotocol/server/HttpServletSseIntegrationTests.java @@ -35,9 +35,7 @@ import org.apache.catalina.LifecycleException; import org.apache.catalina.LifecycleState; import org.apache.catalina.startup.Tomcat; -import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.Timeout; @@ -62,42 +60,17 @@ class HttpServletSseIntegrationTests extends AbstractMcpClientServerIntegrationT protected static final int MAX_REQUEST_SIZE = 2048; - // Tomcat is started once for the whole class; each test swaps in its own transport - private static final TomcatTestUtil.DelegatingServlet MCP_SERVLET = new TomcatTestUtil.DelegatingServlet(); - - private static Tomcat tomcat; - private HttpServletSseServerTransportProvider mcpServerTransportProvider; + // One Tomcat per test, unlike other Servlet tests: the SSE transport may keep writing + // to a stream after its client has disconnected, and with a shared Tomcat these + // writes can land in a connection used by a later test. + private Tomcat tomcat; + static Stream clientsForTesting() { return Stream.of(Arguments.of("httpclient")); } - @BeforeAll - public static void beforeAll() { - tomcat = TomcatTestUtil.createTomcatServer("", PORT, MCP_SERVLET); - try { - tomcat.start(); - assertThat(tomcat.getServer().getState()).isEqualTo(LifecycleState.STARTED); - } - catch (Exception e) { - throw new RuntimeException("Failed to start Tomcat", e); - } - } - - @AfterAll - public static void afterAll() { - if (tomcat != null) { - try { - tomcat.stop(); - tomcat.destroy(); - } - catch (LifecycleException e) { - throw new RuntimeException("Failed to stop Tomcat", e); - } - } - } - @BeforeEach public void before() { // Create and configure the transport provider @@ -107,7 +80,15 @@ public void before() { .sseEndpoint(CUSTOM_SSE_ENDPOINT) .maxRequestSize(MAX_REQUEST_SIZE) .build(); - MCP_SERVLET.setDelegate(mcpServerTransportProvider); + + tomcat = TomcatTestUtil.createTomcatServer("", PORT, mcpServerTransportProvider); + try { + tomcat.start(); + assertThat(tomcat.getServer().getState()).isEqualTo(LifecycleState.STARTED); + } + catch (Exception e) { + throw new RuntimeException("Failed to start Tomcat", e); + } clientBuilders .put("httpclient", @@ -131,6 +112,15 @@ public void after() { if (mcpServerTransportProvider != null) { mcpServerTransportProvider.closeGracefully().block(); } + if (tomcat != null) { + try { + tomcat.stop(); + tomcat.destroy(); + } + catch (LifecycleException e) { + throw new RuntimeException("Failed to stop Tomcat", e); + } + } } @Override diff --git a/mcp-test/src/test/java/io/modelcontextprotocol/server/HttpServletStatelessIntegrationTests.java b/mcp-test/src/test/java/io/modelcontextprotocol/server/HttpServletStatelessIntegrationTests.java index 5cb53b233..0065ea568 100644 --- a/mcp-test/src/test/java/io/modelcontextprotocol/server/HttpServletStatelessIntegrationTests.java +++ b/mcp-test/src/test/java/io/modelcontextprotocol/server/HttpServletStatelessIntegrationTests.java @@ -50,8 +50,8 @@ import org.apache.catalina.LifecycleException; import org.apache.catalina.LifecycleState; import org.apache.catalina.startup.Tomcat; -import org.junit.jupiter.api.AfterAll; -import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.Timeout; import org.junit.jupiter.params.ParameterizedTest; @@ -80,9 +80,9 @@ class HttpServletStatelessIntegrationTests { private static final int MAX_REQUEST_SIZE = 2048; - private static Tomcat tomcat; + private Tomcat tomcat; - private static HttpServletStatelessServerTransport mcpStatelessServerTransport; + private HttpServletStatelessServerTransport mcpStatelessServerTransport; private final McpClient.SyncSpec clientBuilder = McpClient .sync(HttpClientStreamableHttpTransport.builder("http://localhost:" + PORT) @@ -91,8 +91,8 @@ class HttpServletStatelessIntegrationTests { .initializationTimeout(Duration.ofHours(10)) .requestTimeout(Duration.ofHours(10)); - @BeforeAll - public static void beforeAll() { + @BeforeEach + public void before() { mcpStatelessServerTransport = HttpServletStatelessServerTransport.builder() .messageEndpoint(CUSTOM_MESSAGE_ENDPOINT) .maxRequestSize(MAX_REQUEST_SIZE) @@ -108,8 +108,8 @@ public static void beforeAll() { } } - @AfterAll - public static void afterAll() { + @AfterEach + public void after() { if (mcpStatelessServerTransport != null) { mcpStatelessServerTransport.closeGracefully().block(); } diff --git a/mcp-test/src/test/java/io/modelcontextprotocol/server/HttpServletStatelessSharedIntegrationTests.java b/mcp-test/src/test/java/io/modelcontextprotocol/server/HttpServletStatelessSharedIntegrationTests.java index 452eac0de..43e5880f6 100644 --- a/mcp-test/src/test/java/io/modelcontextprotocol/server/HttpServletStatelessSharedIntegrationTests.java +++ b/mcp-test/src/test/java/io/modelcontextprotocol/server/HttpServletStatelessSharedIntegrationTests.java @@ -20,9 +20,7 @@ import org.apache.catalina.LifecycleException; import org.apache.catalina.LifecycleState; import org.apache.catalina.startup.Tomcat; -import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Timeout; import org.junit.jupiter.params.provider.Arguments; @@ -42,10 +40,7 @@ class HttpServletStatelessSharedIntegrationTests extends AbstractStatelessIntegr static McpTransportContextExtractor TEST_CONTEXT_EXTRACTOR = (request) -> McpTransportContext .create(Map.of("important", "value")); - // Tomcat is started once for the whole class; each test swaps in its own transport - private static final TomcatTestUtil.DelegatingServlet MCP_SERVLET = new TomcatTestUtil.DelegatingServlet(); - - private static Tomcat tomcat; + private Tomcat tomcat; private HttpServletStatelessServerTransport mcpServerTransport; @@ -53,38 +48,21 @@ static Stream clientsForTesting() { return Stream.of(Arguments.of("httpclient")); } - @BeforeAll - public static void beforeAll() { - tomcat = TomcatTestUtil.createTomcatServer("", PORT, MCP_SERVLET); - try { - tomcat.start(); - assertThat(tomcat.getServer().getState()).isEqualTo(LifecycleState.STARTED); - } - catch (Exception e) { - throw new RuntimeException("Failed to start Tomcat", e); - } - } - - @AfterAll - public static void afterAll() { - if (tomcat != null) { - try { - tomcat.stop(); - tomcat.destroy(); - } - catch (LifecycleException e) { - throw new RuntimeException("Failed to stop Tomcat", e); - } - } - } - @BeforeEach public void before() { this.mcpServerTransport = HttpServletStatelessServerTransport.builder() .contextExtractor(TEST_CONTEXT_EXTRACTOR) .messageEndpoint(MESSAGE_ENDPOINT) .build(); - MCP_SERVLET.setDelegate(this.mcpServerTransport); + + this.tomcat = TomcatTestUtil.createTomcatServer("", PORT, this.mcpServerTransport); + try { + this.tomcat.start(); + assertThat(this.tomcat.getServer().getState()).isEqualTo(LifecycleState.STARTED); + } + catch (Exception e) { + throw new RuntimeException("Failed to start Tomcat", e); + } prepareClients(PORT, MESSAGE_ENDPOINT); } @@ -112,6 +90,15 @@ public void after() { if (this.mcpServerTransport != null) { this.mcpServerTransport.closeGracefully().block(); } + if (this.tomcat != null) { + try { + this.tomcat.stop(); + this.tomcat.destroy(); + } + catch (LifecycleException e) { + throw new RuntimeException("Failed to stop Tomcat", e); + } + } } } diff --git a/mcp-test/src/test/java/io/modelcontextprotocol/server/HttpServletStreamableIntegrationTests.java b/mcp-test/src/test/java/io/modelcontextprotocol/server/HttpServletStreamableIntegrationTests.java index 0651c0071..ad2a215fe 100644 --- a/mcp-test/src/test/java/io/modelcontextprotocol/server/HttpServletStreamableIntegrationTests.java +++ b/mcp-test/src/test/java/io/modelcontextprotocol/server/HttpServletStreamableIntegrationTests.java @@ -44,9 +44,7 @@ import org.apache.catalina.LifecycleException; import org.apache.catalina.LifecycleState; import org.apache.catalina.startup.Tomcat; -import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.Timeout; @@ -79,10 +77,10 @@ class HttpServletStreamableIntegrationTests extends AbstractMcpClientServerInteg private static final String MESSAGE_ENDPOINT = "/mcp/message"; - // Tomcat is started once for the whole class; each test swaps in its own transport - private static final TomcatTestUtil.DelegatingServlet MCP_SERVLET = new TomcatTestUtil.DelegatingServlet(); + // Some tests swap in another transport, and the servlet tracks the SSE stream + private final TomcatTestUtil.DelegatingServlet mcpServlet = new TomcatTestUtil.DelegatingServlet(); - private static Tomcat tomcat; + private Tomcat tomcat; private HttpServletStreamableServerTransportProvider mcpServerTransportProvider; @@ -102,7 +100,7 @@ class HttpServletStreamableIntegrationTests extends AbstractMcpClientServerInteg protected void awaitClientStreamEstablished() { var timeout = Duration.ofSeconds(5); await().atMost(timeout).untilAsserted(() -> { - assertThat(MCP_SERVLET.isStreamEstablished()) + assertThat(mcpServlet.isStreamEstablished()) .withFailMessage("[Failed to observe MCP Client connection within %s]", timeout) .isTrue(); }); @@ -112,31 +110,6 @@ static Stream clientsForTesting() { return Stream.of(Arguments.of("httpclient")); } - @BeforeAll - public static void beforeAll() { - tomcat = TomcatTestUtil.createTomcatServer("", PORT, MCP_SERVLET); - try { - tomcat.start(); - assertThat(tomcat.getServer().getState()).isEqualTo(LifecycleState.STARTED); - } - catch (Exception e) { - throw new RuntimeException("Failed to start Tomcat", e); - } - } - - @AfterAll - public static void afterAll() { - if (tomcat != null) { - try { - tomcat.stop(); - tomcat.destroy(); - } - catch (LifecycleException e) { - throw new RuntimeException("Failed to stop Tomcat", e); - } - } - } - @BeforeEach public void before() { // Create and configure the transport provider @@ -147,7 +120,16 @@ public void before() { .maxRequestSize(MAX_REQUEST_SIZE) .sessionSweepInterval(SESSION_SWEEP_INTERVAL) .build(); - MCP_SERVLET.setDelegate(mcpServerTransportProvider); + mcpServlet.setDelegate(mcpServerTransportProvider); + + tomcat = TomcatTestUtil.createTomcatServer("", PORT, mcpServlet); + try { + tomcat.start(); + assertThat(tomcat.getServer().getState()).isEqualTo(LifecycleState.STARTED); + } + catch (Exception e) { + throw new RuntimeException("Failed to start Tomcat", e); + } clientBuilders .put("httpclient", @@ -171,6 +153,15 @@ public void after() { if (mcpServerTransportProvider != null) { mcpServerTransportProvider.closeGracefully().block(); } + if (tomcat != null) { + try { + tomcat.stop(); + tomcat.destroy(); + } + catch (LifecycleException e) { + throw new RuntimeException("Failed to stop Tomcat", e); + } + } } @Test @@ -813,7 +804,7 @@ private void withTransportProvider(Duration keepAliveInterval, Duration sessionS // no sweeping: the session must survive every keep-alive round .sessionSweepInterval(sessionSweepInterval) .build(); - MCP_SERVLET.setDelegate(mcpServerTransportProvider); + mcpServlet.setDelegate(mcpServerTransportProvider); prepareAsyncServerBuilder().serverInfo("test-server", "1.0.0").build(); } diff --git a/mcp-test/src/test/java/io/modelcontextprotocol/server/McpCompletionTests.java b/mcp-test/src/test/java/io/modelcontextprotocol/server/McpCompletionTests.java index 377d56c17..1974fe92c 100644 --- a/mcp-test/src/test/java/io/modelcontextprotocol/server/McpCompletionTests.java +++ b/mcp-test/src/test/java/io/modelcontextprotocol/server/McpCompletionTests.java @@ -13,8 +13,8 @@ import org.apache.catalina.LifecycleState; import org.apache.catalina.startup.Tomcat; -import org.junit.jupiter.api.AfterAll; -import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import io.modelcontextprotocol.client.McpClient; @@ -52,15 +52,15 @@ class McpCompletionTests { private static final String CUSTOM_MESSAGE_ENDPOINT = "/otherPath/mcp/message"; - private static HttpServletSseServerTransportProvider mcpServerTransportProvider; + private HttpServletSseServerTransportProvider mcpServerTransportProvider; - private static Tomcat tomcat; + private Tomcat tomcat; final McpClient.SyncSpec clientBuilder = McpClient .sync(HttpClientSseClientTransport.builder("http://localhost:" + PORT).build()); - @BeforeAll - public static void beforeAll() { + @BeforeEach + public void before() { // Create and configure the transport provider mcpServerTransportProvider = HttpServletSseServerTransportProvider.builder() .messageEndpoint(CUSTOM_MESSAGE_ENDPOINT) @@ -76,8 +76,8 @@ public static void beforeAll() { } } - @AfterAll - public static void afterAll() { + @AfterEach + public void after() { if (mcpServerTransportProvider != null) { mcpServerTransportProvider.closeGracefully().block(); } diff --git a/mcp-test/src/test/java/io/modelcontextprotocol/server/McpSyncListFilteringIntegrationTests.java b/mcp-test/src/test/java/io/modelcontextprotocol/server/McpSyncListFilteringIntegrationTests.java index 14b2c0f48..9cadb7a5d 100644 --- a/mcp-test/src/test/java/io/modelcontextprotocol/server/McpSyncListFilteringIntegrationTests.java +++ b/mcp-test/src/test/java/io/modelcontextprotocol/server/McpSyncListFilteringIntegrationTests.java @@ -27,8 +27,6 @@ import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -import org.junit.jupiter.params.AfterParameterizedClassInvocation; -import org.junit.jupiter.params.BeforeParameterizedClassInvocation; import org.junit.jupiter.params.Parameter; import org.junit.jupiter.params.ParameterizedClass; import org.junit.jupiter.params.provider.Arguments; @@ -67,15 +65,13 @@ class McpSyncListFilteringIntegrationTests { }; - @BeforeParameterizedClassInvocation - static void createTransportAndStartTomcat(ServerFactory serverFactory) { + @BeforeEach + void setUp() { + serverFactory.reset(); var port = TomcatTestUtil.findAvailablePort(); baseUrl = "http://localhost:" + port; startTomcat(serverFactory.transport(), port); - } - @BeforeEach - void setUp() { var clientTransport = HttpClientStreamableHttpTransport.builder(baseUrl) .jsonMapper(McpJsonDefaults.getMapper()) .addHttpRequestCustomizer((builder, method, endpoint, body, context) -> requestCustomizer.customize(builder, @@ -90,10 +86,6 @@ void setUp() { @AfterEach void tearDown() { mcpClient.closeGracefully(); - } - - @AfterParameterizedClassInvocation - static void afterAll() { stopTomcat(); } @@ -373,14 +365,21 @@ interface ServerFactory { Servlet transport(); + /** + * Replaces the transport with a fresh one, to start each test from a clean slate. + */ + void reset(); + } static class StatefulServerFactory implements ServerFactory { - private final HttpServletStreamableServerTransportProvider transport = HttpServletStreamableServerTransportProvider - .builder() - .contextExtractor(request -> { + private HttpServletStreamableServerTransportProvider transport; + + @Override + public void reset() { + this.transport = HttpServletStreamableServerTransportProvider.builder().contextExtractor(request -> { var headers = new HashMap(); var names = request.getHeaderNames(); while (names.hasMoreElements()) { @@ -388,8 +387,8 @@ static class StatefulServerFactory implements headers.put(name, request.getHeader(name)); } return McpTransportContext.create(headers); - }) - .build(); + }).build(); + } @Override public ServerBuilderWrapper> syncServer() { @@ -529,8 +528,11 @@ public Servlet transport() { static class StatelessServerFactory implements ServerFactory { - private final HttpServletStatelessServerTransport transport = HttpServletStatelessServerTransport.builder() - .contextExtractor(request -> { + private HttpServletStatelessServerTransport transport; + + @Override + public void reset() { + this.transport = HttpServletStatelessServerTransport.builder().contextExtractor(request -> { var headers = new HashMap(); var names = request.getHeaderNames(); while (names.hasMoreElements()) { @@ -538,8 +540,8 @@ static class StatelessServerFactory implements headers.put(name, request.getHeader(name)); } return McpTransportContext.create(headers); - }) - .build(); + }).build(); + } @Override public ServerBuilderWrapper> syncServer() { diff --git a/mcp-test/src/test/java/io/modelcontextprotocol/server/transport/TomcatTestUtil.java b/mcp-test/src/test/java/io/modelcontextprotocol/server/transport/TomcatTestUtil.java index 84747017d..98e5144e8 100644 --- a/mcp-test/src/test/java/io/modelcontextprotocol/server/transport/TomcatTestUtil.java +++ b/mcp-test/src/test/java/io/modelcontextprotocol/server/transport/TomcatTestUtil.java @@ -66,14 +66,16 @@ public static Tomcat createTomcatServer(String contextPath, int port, Servlet se var connector = tomcat.getConnector(); connector.setAsyncTimeout(3000); + // Bound to loopback: with the wildcard address, stopping Tomcat waits a full + // second for its acceptor to unlock on macOS + connector.setProperty("address", "127.0.0.1"); return tomcat; } /** - * A servlet forwarding all requests to a delegate that can be swapped between tests. - * Register one with {@link #createTomcatServer} to start Tomcat once per test class - * while still handing each test a freshly built transport. + * A servlet forwarding all requests to a delegate that can be swapped during a test, + * and tracking whether a GET SSE stream has been established. */ public static class DelegatingServlet implements Servlet {