#552 #707
#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.
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.
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.
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.
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.
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_processon 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.