-
Notifications
You must be signed in to change notification settings - Fork 600
Ensure that Run tests (http) is actually testing http #3340
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?
Changes from all commits
b35be38
bab62d3
7c70042
f192faa
b64e951
1abdd2b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -242,8 +242,33 @@ public static async Task<Process> StartHttpServerProcessAndWaitForReadinessAsync | |
| output, | ||
| disableAuthentication); | ||
|
|
||
| // Invert: disableAuthentication=false means authenticationEnabled=true | ||
| await WaitForServerReadinessAsync(serverUrl, timeoutSeconds, pollIntervalMs, authenticationEnabled: !disableAuthentication); | ||
| using var readinessCancellation = new CancellationTokenSource(); | ||
| var readinessTask = WaitForServerReadinessAsync( | ||
| serverUrl, | ||
| timeoutSeconds, | ||
| pollIntervalMs, | ||
| authenticationEnabled: !disableAuthentication, | ||
| readinessCancellation.Token); | ||
| var processExitTask = process.WaitForExitAsync(); | ||
|
|
||
|
Comment on lines
+245
to
+253
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This process start code is not using the Use that token as a Leverage You will want to think about what happens if the harnesses cancellation is set and if you need to adjust this code accordingly, e.g., do you need to add code for catching
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using this token in test code is mentioned in https://github.com/microsoft/mcp/blob/main/CONTRIBUTING.md#cancellation-plumbing |
||
| if (await Task.WhenAny(readinessTask, processExitTask) == processExitTask) | ||
| { | ||
| await readinessCancellation.CancelAsync(); | ||
| try | ||
| { | ||
| await readinessTask; | ||
| } | ||
| catch (OperationCanceledException) | ||
| { | ||
| } | ||
|
|
||
| throw new ClientTransportClosedException(new ClientCompletionDetails | ||
| { | ||
| Exception = new InvalidOperationException($"HTTP server process exited with code {process.ExitCode} before becoming ready.") | ||
| }); | ||
| } | ||
|
|
||
| await readinessTask; | ||
|
|
||
| return process; | ||
| } | ||
|
|
@@ -259,7 +284,8 @@ public static async Task WaitForServerReadinessAsync( | |
| string serverUrl, | ||
| int timeoutSeconds = 30, | ||
| int pollIntervalMs = 500, | ||
| bool authenticationEnabled = false) | ||
| bool authenticationEnabled = false, | ||
| CancellationToken cancellationToken = default) | ||
| { | ||
| using var httpClient = new HttpClient(); | ||
| var timeout = TimeSpan.FromSeconds(timeoutSeconds); | ||
|
|
@@ -284,7 +310,7 @@ public static async Task WaitForServerReadinessAsync( | |
| }; | ||
| requestMessage.Headers.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json")); | ||
| requestMessage.Headers.Accept.Add(new MediaTypeWithQualityHeaderValue("text/event-stream")); | ||
| using var resp = await httpClient.SendAsync(requestMessage); | ||
| using var resp = await httpClient.SendAsync(requestMessage, cancellationToken); | ||
|
|
||
| // If authentication is enabled, 401 Unauthorized means server is ready | ||
| // If authentication is disabled, we need a success status code | ||
|
|
@@ -298,7 +324,7 @@ public static async Task WaitForServerReadinessAsync( | |
| { | ||
| // Server not yet available, continue polling | ||
| } | ||
| await Task.Delay(pollIntervalMs); | ||
| await Task.Delay(pollIntervalMs, cancellationToken); | ||
| } | ||
|
|
||
| throw new TimeoutException($"Server at {serverUrl} did not become ready within {timeoutSeconds} seconds"); | ||
|
|
||
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.
Please add a comment in this code that explains why we're treating the modes separately. Your comment may be anecdotal and point to observations of the specific C# MCP SDK version we're seeing this transport-dependent exception behavior with.
Adding this comment will help other maintainers know why this is happening and crucially learn its outside of our control and outside the scope of us understanding. If the C# MCP SDK behavior changes to be consistent between the transport modes, then a contextual comment here will empower someone to make a quick reactive change without needing to investigate the history of this code.
As a general rule, if we ever learn something unintuitive that causes us to conditionalize code, then it's worth documenting in code.
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.
I also think this is worth bringing up with the C# MCP SDK as well. I'd like to learn if this is intentional or not, and if it is intentional, to learn how we might have other error handling scenarios wrong.