test(mcp-test): deflake streamable HTTP version-negotiation integration test - #1113
test(mcp-test): deflake streamable HTTP version-negotiation integration test#1113slachiewicz wants to merge 2 commits into
Conversation
Kehrlann
left a comment
Kehrlann
left a comment
There was a problem hiding this comment.
Thanks for your contribution, please review.
| // The MCP-Protocol-Version header may legitimately be absent on initialize | ||
| // requests, so a missing header must not break context extraction. | ||
| .contextExtractor(req -> McpTransportContext | ||
| .create(Map.of("protocol-version", Objects.requireNonNullElse(req.getHeader("MCP-protocol-version"), "")))) |
There was a problem hiding this comment.
I may be absent, but is it, given the clients we use in this test? If not, leave as-is.
There was a problem hiding this comment.
Correct — the client always sends it on initialize with these clients, so it is never absent here. Reverted to the original extractor in 8fccf33.
This comment was created with AI assistance.
| // The GET /mcp stream is opened asynchronously once the initialize response | ||
| // creates the session, so wait for it to be recorded before asserting. | ||
| await().atMost(Duration.ofSeconds(5)) | ||
| .untilAsserted(() -> assertThat(requestRecordingFilter.getCalls()).filteredOn(c -> "GET".equals(c.method())) | ||
| .hasSize(1)); | ||
|
|
There was a problem hiding this comment.
Wrap the whole assertion below (calls = ... + assertThat(calls)) in the await(), and include your comment there.
There was a problem hiding this comment.
Done in 8fccf33 — the whole assertion now runs inside await(), comment moved with it.
This comment was created with AI assistance.
|
The Jackson 2 job on 8fccf33 failed in This comment was created with AI assistance. |
…on test - tolerate an absent MCP-Protocol-Version header in the context extractor: Map.of rejects null values while spec-correct clients legitimately omit the header on initialize requests - await the asynchronously opened GET /mcp stream before asserting recorded calls; observed intermittently in CI as AssertionError Expected size: 3 but was: 2 (evidence: actions/run/33069467622 Jackson 2 Integration Tests)
…actor Review follow-up: the header is never absent with the clients used here, so the contextExtractor stays as on main; the whole size-3 assertion now runs inside await() so a late POST is covered as well as the async GET. Claude-Session: https://claude.ai/code/session_01WtjUifqJuzJdfxc1zjcZrL
926b8c1 to
d77e9f7
Compare
2 participants
Two independent timing hazards cause intermittent CI failures:
Change: null-safe extractor (Objects.requireNonNullElse) + Awaitility guard for the recorded GET.
Tested: ./mvnw -pl mcp-test -am test -Dtest=HttpClientStreamableHttpVersionNegotiationIntegrationTests → 2/2 green; no production code touched.