Skip to content
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

Improve client test reliability and execution time #52

Merged
merged 1 commit into from
Mar 19, 2025

Conversation

chemicL
Copy link
Member

@chemicL chemicL commented Mar 19, 2025

This change uses VirtualTimeScheduler and pretends enough time has passed to trigger a timeout on the initialization.

Another problem with reliability of the tests was that the used testcontainer for the SSE server does not support multiple clients and the existence of both the global client for the entire suite and some customized local clients in some tests caused responses to be delivered to the other client at some racing situations. Now each test creates a dedicated client and performs cleanup locally.

While these tests were improved, two other issues were found and fixed:

  • The first one is that the closeGracefully of DefaultMcpSession was not lazy and would trigger connection disposal before the returned Mono was subscribed.
  • The second one was dealing with closing the StdIo client before the process was started. In such a case there should not be an error but rather a warning and successful completion.

This change uses VirtualTimeScheduler and pretends enough time has
passed to trigger a timeout on the initialization.
Another problem with reliability of the tests was that the used
testcontainer for the SSE server does not support multiple clients and
the existence of both the global client for the entire suite and some
customized local clients in some tests caused responses to be delivered
to the other client at some racing situations. Now each test creates a
dedicated client and performs cleanup locally.
While these tests were improved, two other issues were found and fixed.
The first one is that the closeGracefully of DefaultMcpSession was not
lazy and would trigger connection disposal before the returned Mono was
subscribed. The second one was dealing with closing the StdIo client
before the process was started. In such a case there should not be an
error but rather a warning and successful completion.
@chemicL chemicL force-pushed the client-tests-reliability branch from 630a126 to da548f9 Compare March 19, 2025 16:30
Copy link
Contributor

@tzolov tzolov left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks @chemicL
Nice catch for the closeGracefully and the StdIo closing.

@tzolov tzolov added this to the 0.8.0 milestone Mar 19, 2025
@tzolov tzolov merged commit 37120f2 into main Mar 19, 2025
1 check passed
@tzolov tzolov deleted the client-tests-reliability branch March 19, 2025 17:33
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