Skip to content

Commit 88ebcb7

Browse files
KehrlannKehrlann
authored
Servlet tests: use one Tomcat per test (#1309)
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 <git@garnier.wf>
1 parent e7d8a36 commit 88ebcb7

7 files changed

Lines changed: 109 additions & 137 deletions

‎mcp-test/src/test/java/io/modelcontextprotocol/server/HttpServletSseIntegrationTests.java‎

Lines changed: 23 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -35,9 +35,7 @@
3535
import org.apache.catalina.LifecycleException;
3636
import org.apache.catalina.LifecycleState;
3737
import org.apache.catalina.startup.Tomcat;
38-
import org.junit.jupiter.api.AfterAll;
3938
import org.junit.jupiter.api.AfterEach;
40-
import org.junit.jupiter.api.BeforeAll;
4139
import org.junit.jupiter.api.BeforeEach;
4240
import org.junit.jupiter.api.Test;
4341
import org.junit.jupiter.api.Timeout;
@@ -62,42 +60,17 @@ class HttpServletSseIntegrationTests extends AbstractMcpClientServerIntegrationT
6260

6361
protected static final int MAX_REQUEST_SIZE = 2048;
6462

65-
// Tomcat is started once for the whole class; each test swaps in its own transport
66-
private static final TomcatTestUtil.DelegatingServlet MCP_SERVLET = new TomcatTestUtil.DelegatingServlet();
67-
68-
private static Tomcat tomcat;
69-
7063
private HttpServletSseServerTransportProvider mcpServerTransportProvider;
7164

65+
// One Tomcat per test, unlike other Servlet tests: the SSE transport may keep writing
66+
// to a stream after its client has disconnected, and with a shared Tomcat these
67+
// writes can land in a connection used by a later test.
68+
private Tomcat tomcat;
69+
7270
static Stream<Arguments> clientsForTesting() {
7371
return Stream.of(Arguments.of("httpclient"));
7472
}
7573

76-
@BeforeAll
77-
public static void beforeAll() {
78-
tomcat = TomcatTestUtil.createTomcatServer("", PORT, MCP_SERVLET);
79-
try {
80-
tomcat.start();
81-
assertThat(tomcat.getServer().getState()).isEqualTo(LifecycleState.STARTED);
82-
}
83-
catch (Exception e) {
84-
throw new RuntimeException("Failed to start Tomcat", e);
85-
}
86-
}
87-
88-
@AfterAll
89-
public static void afterAll() {
90-
if (tomcat != null) {
91-
try {
92-
tomcat.stop();
93-
tomcat.destroy();
94-
}
95-
catch (LifecycleException e) {
96-
throw new RuntimeException("Failed to stop Tomcat", e);
97-
}
98-
}
99-
}
100-
10174
@BeforeEach
10275
public void before() {
10376
// Create and configure the transport provider
@@ -107,7 +80,15 @@ public void before() {
10780
.sseEndpoint(CUSTOM_SSE_ENDPOINT)
10881
.maxRequestSize(MAX_REQUEST_SIZE)
10982
.build();
110-
MCP_SERVLET.setDelegate(mcpServerTransportProvider);
83+
84+
tomcat = TomcatTestUtil.createTomcatServer("", PORT, mcpServerTransportProvider);
85+
try {
86+
tomcat.start();
87+
assertThat(tomcat.getServer().getState()).isEqualTo(LifecycleState.STARTED);
88+
}
89+
catch (Exception e) {
90+
throw new RuntimeException("Failed to start Tomcat", e);
91+
}
11192

11293
clientBuilders
11394
.put("httpclient",
@@ -131,6 +112,15 @@ public void after() {
131112
if (mcpServerTransportProvider != null) {
132113
mcpServerTransportProvider.closeGracefully().block();
133114
}
115+
if (tomcat != null) {
116+
try {
117+
tomcat.stop();
118+
tomcat.destroy();
119+
}
120+
catch (LifecycleException e) {
121+
throw new RuntimeException("Failed to stop Tomcat", e);
122+
}
123+
}
134124
}
135125

136126
@Override

‎mcp-test/src/test/java/io/modelcontextprotocol/server/HttpServletStatelessIntegrationTests.java‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -50,8 +50,8 @@
5050
import org.apache.catalina.LifecycleException;
5151
import org.apache.catalina.LifecycleState;
5252
import org.apache.catalina.startup.Tomcat;
53-
import org.junit.jupiter.api.AfterAll;
54-
import org.junit.jupiter.api.BeforeAll;
53+
import org.junit.jupiter.api.AfterEach;
54+
import org.junit.jupiter.api.BeforeEach;
5555
import org.junit.jupiter.api.Test;
5656
import org.junit.jupiter.api.Timeout;
5757
import org.junit.jupiter.params.ParameterizedTest;
@@ -80,9 +80,9 @@ class HttpServletStatelessIntegrationTests {
8080

8181
private static final int MAX_REQUEST_SIZE = 2048;
8282

83-
private static Tomcat tomcat;
83+
private Tomcat tomcat;
8484

85-
private static HttpServletStatelessServerTransport mcpStatelessServerTransport;
85+
private HttpServletStatelessServerTransport mcpStatelessServerTransport;
8686

8787
private final McpClient.SyncSpec clientBuilder = McpClient
8888
.sync(HttpClientStreamableHttpTransport.builder("http://localhost:" + PORT)
@@ -91,8 +91,8 @@ class HttpServletStatelessIntegrationTests {
9191
.initializationTimeout(Duration.ofHours(10))
9292
.requestTimeout(Duration.ofHours(10));
9393

94-
@BeforeAll
95-
public static void beforeAll() {
94+
@BeforeEach
95+
public void before() {
9696
mcpStatelessServerTransport = HttpServletStatelessServerTransport.builder()
9797
.messageEndpoint(CUSTOM_MESSAGE_ENDPOINT)
9898
.maxRequestSize(MAX_REQUEST_SIZE)
@@ -108,8 +108,8 @@ public static void beforeAll() {
108108
}
109109
}
110110

111-
@AfterAll
112-
public static void afterAll() {
111+
@AfterEach
112+
public void after() {
113113
if (mcpStatelessServerTransport != null) {
114114
mcpStatelessServerTransport.closeGracefully().block();
115115
}

‎mcp-test/src/test/java/io/modelcontextprotocol/server/HttpServletStatelessSharedIntegrationTests.java‎

Lines changed: 19 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -20,9 +20,7 @@
2020
import org.apache.catalina.LifecycleException;
2121
import org.apache.catalina.LifecycleState;
2222
import org.apache.catalina.startup.Tomcat;
23-
import org.junit.jupiter.api.AfterAll;
2423
import org.junit.jupiter.api.AfterEach;
25-
import org.junit.jupiter.api.BeforeAll;
2624
import org.junit.jupiter.api.BeforeEach;
2725
import org.junit.jupiter.api.Timeout;
2826
import org.junit.jupiter.params.provider.Arguments;
@@ -42,49 +40,29 @@ class HttpServletStatelessSharedIntegrationTests extends AbstractStatelessIntegr
4240
static McpTransportContextExtractor<HttpServletRequest> TEST_CONTEXT_EXTRACTOR = (request) -> McpTransportContext
4341
.create(Map.of("important", "value"));
4442

45-
// Tomcat is started once for the whole class; each test swaps in its own transport
46-
private static final TomcatTestUtil.DelegatingServlet MCP_SERVLET = new TomcatTestUtil.DelegatingServlet();
47-
48-
private static Tomcat tomcat;
43+
private Tomcat tomcat;
4944

5045
private HttpServletStatelessServerTransport mcpServerTransport;
5146

5247
static Stream<Arguments> clientsForTesting() {
5348
return Stream.of(Arguments.of("httpclient"));
5449
}
5550

56-
@BeforeAll
57-
public static void beforeAll() {
58-
tomcat = TomcatTestUtil.createTomcatServer("", PORT, MCP_SERVLET);
59-
try {
60-
tomcat.start();
61-
assertThat(tomcat.getServer().getState()).isEqualTo(LifecycleState.STARTED);
62-
}
63-
catch (Exception e) {
64-
throw new RuntimeException("Failed to start Tomcat", e);
65-
}
66-
}
67-
68-
@AfterAll
69-
public static void afterAll() {
70-
if (tomcat != null) {
71-
try {
72-
tomcat.stop();
73-
tomcat.destroy();
74-
}
75-
catch (LifecycleException e) {
76-
throw new RuntimeException("Failed to stop Tomcat", e);
77-
}
78-
}
79-
}
80-
8151
@BeforeEach
8252
public void before() {
8353
this.mcpServerTransport = HttpServletStatelessServerTransport.builder()
8454
.contextExtractor(TEST_CONTEXT_EXTRACTOR)
8555
.messageEndpoint(MESSAGE_ENDPOINT)
8656
.build();
87-
MCP_SERVLET.setDelegate(this.mcpServerTransport);
57+
58+
this.tomcat = TomcatTestUtil.createTomcatServer("", PORT, this.mcpServerTransport);
59+
try {
60+
this.tomcat.start();
61+
assertThat(this.tomcat.getServer().getState()).isEqualTo(LifecycleState.STARTED);
62+
}
63+
catch (Exception e) {
64+
throw new RuntimeException("Failed to start Tomcat", e);
65+
}
8866

8967
prepareClients(PORT, MESSAGE_ENDPOINT);
9068
}
@@ -112,6 +90,15 @@ public void after() {
11290
if (this.mcpServerTransport != null) {
11391
this.mcpServerTransport.closeGracefully().block();
11492
}
93+
if (this.tomcat != null) {
94+
try {
95+
this.tomcat.stop();
96+
this.tomcat.destroy();
97+
}
98+
catch (LifecycleException e) {
99+
throw new RuntimeException("Failed to stop Tomcat", e);
100+
}
101+
}
115102
}
116103

117104
}

‎mcp-test/src/test/java/io/modelcontextprotocol/server/HttpServletStreamableIntegrationTests.java‎

Lines changed: 24 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -44,9 +44,7 @@
4444
import org.apache.catalina.LifecycleException;
4545
import org.apache.catalina.LifecycleState;
4646
import org.apache.catalina.startup.Tomcat;
47-
import org.junit.jupiter.api.AfterAll;
4847
import org.junit.jupiter.api.AfterEach;
49-
import org.junit.jupiter.api.BeforeAll;
5048
import org.junit.jupiter.api.BeforeEach;
5149
import org.junit.jupiter.api.Test;
5250
import org.junit.jupiter.api.Timeout;
@@ -79,10 +77,10 @@ class HttpServletStreamableIntegrationTests extends AbstractMcpClientServerInteg
7977

8078
private static final String MESSAGE_ENDPOINT = "/mcp/message";
8179

82-
// Tomcat is started once for the whole class; each test swaps in its own transport
83-
private static final TomcatTestUtil.DelegatingServlet MCP_SERVLET = new TomcatTestUtil.DelegatingServlet();
80+
// Some tests swap in another transport, and the servlet tracks the SSE stream
81+
private final TomcatTestUtil.DelegatingServlet mcpServlet = new TomcatTestUtil.DelegatingServlet();
8482

85-
private static Tomcat tomcat;
83+
private Tomcat tomcat;
8684

8785
private HttpServletStreamableServerTransportProvider mcpServerTransportProvider;
8886

@@ -102,7 +100,7 @@ class HttpServletStreamableIntegrationTests extends AbstractMcpClientServerInteg
102100
protected void awaitClientStreamEstablished() {
103101
var timeout = Duration.ofSeconds(5);
104102
await().atMost(timeout).untilAsserted(() -> {
105-
assertThat(MCP_SERVLET.isStreamEstablished())
103+
assertThat(mcpServlet.isStreamEstablished())
106104
.withFailMessage("[Failed to observe MCP Client connection within %s]", timeout)
107105
.isTrue();
108106
});
@@ -112,31 +110,6 @@ static Stream<Arguments> clientsForTesting() {
112110
return Stream.of(Arguments.of("httpclient"));
113111
}
114112

115-
@BeforeAll
116-
public static void beforeAll() {
117-
tomcat = TomcatTestUtil.createTomcatServer("", PORT, MCP_SERVLET);
118-
try {
119-
tomcat.start();
120-
assertThat(tomcat.getServer().getState()).isEqualTo(LifecycleState.STARTED);
121-
}
122-
catch (Exception e) {
123-
throw new RuntimeException("Failed to start Tomcat", e);
124-
}
125-
}
126-
127-
@AfterAll
128-
public static void afterAll() {
129-
if (tomcat != null) {
130-
try {
131-
tomcat.stop();
132-
tomcat.destroy();
133-
}
134-
catch (LifecycleException e) {
135-
throw new RuntimeException("Failed to stop Tomcat", e);
136-
}
137-
}
138-
}
139-
140113
@BeforeEach
141114
public void before() {
142115
// Create and configure the transport provider
@@ -147,7 +120,16 @@ public void before() {
147120
.maxRequestSize(MAX_REQUEST_SIZE)
148121
.sessionSweepInterval(SESSION_SWEEP_INTERVAL)
149122
.build();
150-
MCP_SERVLET.setDelegate(mcpServerTransportProvider);
123+
mcpServlet.setDelegate(mcpServerTransportProvider);
124+
125+
tomcat = TomcatTestUtil.createTomcatServer("", PORT, mcpServlet);
126+
try {
127+
tomcat.start();
128+
assertThat(tomcat.getServer().getState()).isEqualTo(LifecycleState.STARTED);
129+
}
130+
catch (Exception e) {
131+
throw new RuntimeException("Failed to start Tomcat", e);
132+
}
151133

152134
clientBuilders
153135
.put("httpclient",
@@ -171,6 +153,15 @@ public void after() {
171153
if (mcpServerTransportProvider != null) {
172154
mcpServerTransportProvider.closeGracefully().block();
173155
}
156+
if (tomcat != null) {
157+
try {
158+
tomcat.stop();
159+
tomcat.destroy();
160+
}
161+
catch (LifecycleException e) {
162+
throw new RuntimeException("Failed to stop Tomcat", e);
163+
}
164+
}
174165
}
175166

176167
@Test
@@ -813,7 +804,7 @@ private void withTransportProvider(Duration keepAliveInterval, Duration sessionS
813804
// no sweeping: the session must survive every keep-alive round
814805
.sessionSweepInterval(sessionSweepInterval)
815806
.build();
816-
MCP_SERVLET.setDelegate(mcpServerTransportProvider);
807+
mcpServlet.setDelegate(mcpServerTransportProvider);
817808
prepareAsyncServerBuilder().serverInfo("test-server", "1.0.0").build();
818809
}
819810

‎mcp-test/src/test/java/io/modelcontextprotocol/server/McpCompletionTests.java‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,8 @@
1313
import org.apache.catalina.LifecycleState;
1414
import org.apache.catalina.startup.Tomcat;
1515

16-
import org.junit.jupiter.api.AfterAll;
17-
import org.junit.jupiter.api.BeforeAll;
16+
import org.junit.jupiter.api.AfterEach;
17+
import org.junit.jupiter.api.BeforeEach;
1818
import org.junit.jupiter.api.Test;
1919

2020
import io.modelcontextprotocol.client.McpClient;
@@ -52,15 +52,15 @@ class McpCompletionTests {
5252

5353
private static final String CUSTOM_MESSAGE_ENDPOINT = "/otherPath/mcp/message";
5454

55-
private static HttpServletSseServerTransportProvider mcpServerTransportProvider;
55+
private HttpServletSseServerTransportProvider mcpServerTransportProvider;
5656

57-
private static Tomcat tomcat;
57+
private Tomcat tomcat;
5858

5959
final McpClient.SyncSpec clientBuilder = McpClient
6060
.sync(HttpClientSseClientTransport.builder("http://localhost:" + PORT).build());
6161

62-
@BeforeAll
63-
public static void beforeAll() {
62+
@BeforeEach
63+
public void before() {
6464
// Create and configure the transport provider
6565
mcpServerTransportProvider = HttpServletSseServerTransportProvider.builder()
6666
.messageEndpoint(CUSTOM_MESSAGE_ENDPOINT)
@@ -76,8 +76,8 @@ public static void beforeAll() {
7676
}
7777
}
7878

79-
@AfterAll
80-
public static void afterAll() {
79+
@AfterEach
80+
public void after() {
8181
if (mcpServerTransportProvider != null) {
8282
mcpServerTransportProvider.closeGracefully().block();
8383
}

0 commit comments

Comments
 (0)