-
Notifications
You must be signed in to change notification settings - Fork 2k
#552 #707
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
#552 #707
Conversation
Refactored test to utilize `stdio_client` for more accurate simulation of the fixed behavior and added handling for proper initialization with `ClientSession`. Increased timeout to 10 seconds to prevent premature failures and ensured hanging issue is properly caught and reported. Signed-off-by: DanielAvdar <[email protected]>
…ptions Signed-off-by: DanielAvdar <[email protected]>
The test |
…d exceptions Signed-off-by: DanielAvdar <[email protected]>
Signed-off-by: DanielAvdar <[email protected]>
Updated the Windows-specific test to use `@pytest.mark.parametrize` for varied command arguments. This change improves test coverage and simplifies the testing of process creation scenarios. Signed-off-by: DanielAvdar <[email protected]>
Signed-off-by: DanielAvdar <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @DanielAvdar for submitting this PR and helping us improve our test coverage!
I've left some comments on the PR - please address these and I'm happy to review quickly so we can get this landed.
from mcp import ClientSession, StdioServerParameters | ||
from mcp.client.stdio import stdio_client | ||
|
||
# @pytest.mark.skipif(sys.platform != "win32", reason="Windows-specific test") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be uncommented? Tests do seem to fail on Ubuntu
# Directly test the fixed function that was causing the hanging issue | ||
try: | ||
# Set a timeout to prevent hanging | ||
async with asyncio.timeout(5): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use anyio.timeout
or anyio.fail_after
here please to align with other tests?
from mcp.client.stdio import stdio_client | ||
|
||
# @pytest.mark.skipif(sys.platform != "win32", reason="Windows-specific test") | ||
@pytest.mark.skipif(sys.version_info < (3, 11), reason="asyncio.timeout in 3.11+") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you switch to anyio
like recommended below, you shouldn't need this skip case anymore.
# Directly test the fixed function that was causing the hanging issue | ||
try: | ||
# Set a timeout to prevent hanging | ||
async with asyncio.timeout(5): | ||
# Test the actual process creation function that was fixed | ||
async with stdio_client(params) as (read, write): | ||
print("inside client") | ||
async with ClientSession(read, write) as c: | ||
print("inside ClientSession") | ||
await c.initialize() | ||
|
||
except asyncio.TimeoutError: | ||
pytest.xfail("Process creation timed out, indicating a hang issue") | ||
except ProcessLookupError: | ||
pytest.xfail("Process creation failed with ProcessLookupError") | ||
except Exception as e: | ||
assert "ExceptionGroup" in repr(e), f"Unexpected error: {e}" | ||
assert "ProcessLookupError" in repr(e), f"Unexpected error: {e}" | ||
pytest.xfail(f"Expected error: {e}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you simplify to use anyio.fail_after(5)
like so?
# Will fail with TimeoutError if it hangs
with anyio.fail_after(5):
async with stdio_client(params) as (read, write):
print("inside client")
async with ClientSession(read, write) as c:
print("inside ClientSession")
await c.initialize()
Then you don't need to do all the except
statements either.
This pull request adds a new test to verify the fix for a Windows-specific process creation issue described in issue #552. The test ensures that the process creation function works without hanging.
Added test for Windows process creation:
tests/issues/test_552_windows_hang.py
: Introduced a new test,test_windows_process_creation
, to validate the functionality of_create_platform_compatible_process
on Windows. The test uses a simple command (cmd /c echo Test successful
) and sets a timeout to prevent hanging. It verifies that the process is created successfully and produces output.