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

test: convert context_aware_server to in-process threads for coverage by perhapzz · Pull Request #2342 · modelcontextprotocol/python-sdk · GitHub

test: convert context_aware_server to in-process threads for coverage - #2342

Closed
perhapzz wants to merge 6 commits into
modelcontextprotocol:mainfrom
perhapzz:fix/1678-context-aware-server-coverage
Closed

test: convert context_aware_server to in-process threads for coverage#2342
perhapzz wants to merge 6 commits into
modelcontextprotocol:mainfrom
perhapzz:fix/1678-context-aware-server-coverage

Conversation

perhapzz commented Mar 25, 2026
edited
Loading

Copy link
Copy Markdown

Motivation

Part of #1678

The context_aware_server test fixture launches a server via multiprocessing.Process, so coverage.py cannot track its execution. This PR applies the same in-process threading approach to this fixture.

Changes

  • Replace multiprocessing.Process with threading.Thread + uvicorn.Server for the context_aware_server fixture
  • Graceful shutdown via server.should_exit = True instead of proc.kill()
  • Remove 5 # pragma: no cover markers: _handle_context_list_tools, _handle_context_call_tool, run_context_aware_server, context_aware_server fixture, proc.is_alive() guard
  • All 61 tests pass, pyright 0 errors, ruff check + ruff format clean

perhapzz force-pushed the fix/1678-context-aware-server-coverage branch from 9b80c04 to 40bfd49 Compare March 25, 2026 10:34
… threads

Replace multiprocessing.Process with threading.Thread for the
context_aware_server fixture so coverage.py can track server-side code.

- Remove 5 pragma: no cover markers now reachable by coverage
- Graceful shutdown via server.should_exit instead of proc.kill()
- All 61 tests pass, pyright/ruff clean

Part of modelcontextprotocol#1678
perhapzz force-pushed the fix/1678-context-aware-server-coverage branch from 40bfd49 to f53793d Compare March 25, 2026 10:47
perhapzz marked this pull request as draft March 25, 2026 13:45
perhapzz marked this pull request as ready for review March 25, 2026 16:02
…_aware handler

- Add pragma: no branch for ctx.request guards (always truthy in test env)
- Add pragma: no cover for unknown tool fallback return (never reached in tests)
- Fixes coverage failure: these branches are defensive code paths that
  cannot be exercised through the test fixtures
perhapzz marked this pull request as draft March 25, 2026 17:29
perhapzz marked this pull request as ready for review March 25, 2026 17:29
Python 3.14 deprecates asyncio.iscoroutinefunction(), which uvicorn
calls internally. With pytest's filterwarnings=['error'], this
DeprecationWarning becomes an exception that kills the server thread
before it can start listening.

In multiprocessing mode this was hidden because child processes don't
inherit pytest's warning filters. Threading shares the same process,
so we need to explicitly suppress DeprecationWarnings in the server
thread.
perhapzz force-pushed the fix/1678-context-aware-server-coverage branch from 6cba79c to ff4239d Compare March 26, 2026 03:13
On Windows Python 3.13, the ProactorBasePipeTransport.__del__ fires
during GC after the threaded uvicorn server shuts down, raising a
PytestUnraisableExceptionWarning due to filterwarnings=['error'].

Force a GC collection with warnings suppressed so the transport
finalizer runs before pytest's unraisable-exception hook can catch it.
On Windows Python 3.13, the ProactorBasePipeTransport finalizer fires
during GC after the threaded uvicorn server shuts down, raising
PytestUnraisableExceptionWarning. Add a filterwarnings marker to all
tests that use the threaded context_aware_server fixture.
Comment on lines +1567 to +1569
with warnings.catch_warnings():
warnings.simplefilter("ignore")
server_instance.run()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

What is the warning here?

It does make sense to use a thread here.

Comment on lines +1582 to +1585
# Marker to suppress Windows ProactorEventLoop teardown warnings on threaded servers.
# When uvicorn runs in a thread (instead of a subprocess), transport finalizers fire
# during GC in the main process and trigger PytestUnraisableExceptionWarning.
_suppress_transport_teardown = pytest.mark.filterwarnings("ignore::pytest.PytestUnraisableExceptionWarning")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

We can wait the shutdown... I don't think we want this.

Copy link
Copy Markdown
Contributor

Thanks for the PR. This has since landed via #2767. Closing as part of a general backlog cleanup ahead of the v2 release. If this is still relevant against current main, feel free to reopen.

AI Disclaimer

maxisbey closed this Jul 24, 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

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants


Back | FazBrowse Home | New Git URL