Skip to content

tests: avoid racy HTTP test ports#2760

Draft
tarunag10 wants to merge 4 commits into
modelcontextprotocol:mainfrom
tarunag10:codex/fix-python-sdk-port-race
Draft

tests: avoid racy HTTP test ports#2760
tarunag10 wants to merge 4 commits into
modelcontextprotocol:mainfrom
tarunag10:codex/fix-python-sdk-port-race

Conversation

@tarunag10
Copy link
Copy Markdown

Summary

  • Replaces racy pick-free-port-then-rebind server fixtures in the streamable HTTP/SSE test files with the existing run_uvicorn_in_thread helper.
  • Updates affected tests to consume URLs yielded from the actual listening uvicorn socket instead of separately selected port integers.
  • Adds a regression assertion that the basic streamable HTTP fixture URL uses a port that is already reserved, and closes the helper socket explicitly during shutdown.

This is an alternative/focused fix for #2704 using the existing thread helper. I noticed #2705 is already open with a process-helper approach; this draft is opened for maintainer comparison rather than as a duplicate ready-to-merge submission.

Validation

  • uv run ruff check tests/test_helpers.py tests/shared/test_streamable_http.py tests/shared/test_sse.py tests/server/test_sse_security.py tests/server/test_streamable_http_security.py tests/client/test_http_unicode.py
  • uv run pytest tests/shared/test_streamable_http.py tests/shared/test_sse.py tests/server/test_sse_security.py tests/server/test_streamable_http_security.py tests/client/test_http_unicode.py -> 120 passed, 1 skipped
  • uv run pytest -n auto tests/shared/test_streamable_http.py tests/shared/test_sse.py tests/server/test_sse_security.py tests/server/test_streamable_http_security.py tests/client/test_http_unicode.py -> first run had one non-port timing failure in test_streamable_http_multiple_reconnections; isolated rerun passed; second full affected xdist run passed with 120 passed, 1 skipped

@StantonMatt
Copy link
Copy Markdown

I looked at the red CI on 9dd8080. The failures do not look like assertion failures from the port changes.

The two representative jobs I opened, checks / test (3.10, locked, ubuntu-latest) and checks / test (3.10, locked, windows-latest), both finished pytest successfully and then failed during coverage report:

  • Ubuntu: 1743 passed, 98 skipped, 1 xfailed, then Coverage failure: total of 99.99 is less than fail-under=100.00
  • Windows: 1742 passed, 99 skipped, 1 xfailed, then the same 99.99 < 100.00 coverage failure

The missing coverage report points at src/mcp/server/streamable_http.py lines 1020-1022, plus the two test security helper classes. That lines up with the last commit removing # pragma: no cover from the closed-stream exception cleanup path and those test helpers.

So I think the narrow fix is either to add coverage for those newly-counted paths, or leave the no-cover pragmas on branches/helpers that are intentionally not reachable in this test setup. I also ran git diff --check origin/main...HEAD locally on this head and it passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants