FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: update SSE tests to use StreamingASGITransport instead of uvicorn for server testing by bjoaquinc · Pull Request #1566 · modelcontextprotocol/python-sdk · GitHub

test: update SSE tests to use StreamingASGITransport instead of uvicorn for server testing - #1566

Closed
bjoaquinc wants to merge 24 commits into
modelcontextprotocol:mainfrom
bjoaquinc:streaming-asgi-sse
Closed

test: update SSE tests to use StreamingASGITransport instead of uvicorn for server testing#1566
bjoaquinc wants to merge 24 commits into
modelcontextprotocol:mainfrom
bjoaquinc:streaming-asgi-sse

Conversation

bjoaquinc commented Nov 3, 2025
edited
Loading

Copy link
Copy Markdown

This implements fixes to the StreamingASGITransport to properly handle SSE streaming and disconnect signaling. The key changes were:

  • Added disconnect signaling via receive() to fix SSE hanging
  • Fixed memory stream cleanup in SseServerTransport to eliminate ResourceWarnings
  • Handle duplicate http.response.start with NoopASGI
  • Fixed global sse-starlette quirk
  • Update all tests in tests/server/test_sse_security.py and tests/shared/test_sse.py to use transports instead of real servers with uvicorn

Motivation and Context

Partially resolves #857

How Has This Been Tested?

Since I updated the testing methodology I made sure to run the test in batches of 30 (to check for race conditions). Using the command:

passed=0; failed=0; for i in {1..30}; do echo "=== Run $i/30 ===" && uv run --resolution="$RESOLUTION" pytest "$FILE_PATH" -q && passed=$((passed+1)) || failed=$((failed+1)); done; echo "==== SUMMARY: $passed passed, $failed failed out of 30 runs ===="

for the following resolutions and files:

  • tests/server/test_sse_security.py for lowest-direct and highest
  • tests/shared/test_sse.py for lowest-direct and highest

All tests passed:

Breaking Changes

None. StreamingASGITransport is only meant for testing. The only change I made to SseServerTransport was to properly clean up sse_stream_reader which is best practice and doesn't break functionality. The modified tests all function as expected.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

maxisbey added P3 Nice to haves, rare edge cases needs more work Not ready to be merged yet, needs additional follow-up from the author(s). labels Nov 4, 2025

Copy link
Copy Markdown
Author

Done with all updates and ready for a review.

Copy link
Copy Markdown
Contributor

Related: #1728 (v2 test client improvements)

AI Disclaimer

This comment was marked as abuse.

Copy link
Copy Markdown
Contributor

Thanks for the PR. This has since landed via #2765. Closing this as part of a wider backlog cleanup following the v2 launch. If it's still relevant, feel free to reopen.

AI Disclaimer

maxisbey closed this Jul 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs more work Not ready to be merged yet, needs additional follow-up from the author(s). P3 Nice to haves, rare edge cases

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Drop uvicorn from test suite

3 participants


Back | FazBrowse Home | New Git URL