Ensure that Run tests (http) is actually testing http - #3340
Ensure that Run tests (http) is actually testing http#3340Chidozie Ononiwu (chidozieononiwu) wants to merge 6 commits into
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This PR updates the Azure Pipelines live-test job template to ensure the “Run tests (http)” step actually executes the test suite using the HTTP transport, aligning the pipeline behavior with the step’s intent and label.
Changes:
- Sets
MCP_TEST_TRANSPORTtohttpfor the “Run tests (http) - az pwsh” task to force HTTP-based test transport selection during that run.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
/azp run mcp - pullrequest - live |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
b65993f to
ecba734
Compare
|
/azp run mcp - pullrequest - live |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
ecba734 to
f195bdf
Compare
|
/azp run mcp - pullrequest - live |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run mcp - pullrequest - live |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
f27d2a0 to
37f2858
Compare
|
/azp run mcp - pullrequest - live |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
37f2858 to
1abdd2b
Compare
|
/azp run mcp - pullrequest - live |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
| "prompts/get"); | ||
| } | ||
|
|
||
| private static async Task AssertMethodNotFoundAsync(Func<Task> action, string method) |
There was a problem hiding this comment.
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.
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.
| using var readinessCancellation = new CancellationTokenSource(); | ||
| var readinessTask = WaitForServerReadinessAsync( | ||
| serverUrl, | ||
| timeoutSeconds, | ||
| pollIntervalMs, | ||
| authenticationEnabled: !disableAuthentication, | ||
| readinessCancellation.Token); | ||
| var processExitTask = process.WaitForExitAsync(); | ||
|
|
There was a problem hiding this comment.
This process start code is not using the Xunit.TestContext.Current.CancellationToken.
Use that token as a CancellationToken to process.WaitForExitAsync();
Leverage CancellationTokenSource.CreateLinkedTokenSource() when making new CancellationTokenSources. using var readinessCancellation = CancellationTokenSource.CreateLinkedTokenSource(TestContext.Current.CancellationToken);
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 TaskCanceledException here or let it bubble up?
There was a problem hiding this comment.
Using this token in test code is mentioned in https://github.com/microsoft/mcp/blob/main/CONTRIBUTING.md#cancellation-plumbing
This pull request improves test reliability and error handling for unsupported operations when running in HTTP transport mode. It introduces a shared assertion helper to standardize checks for "method not found" errors, updates tests to use this helper, and ensures that tool command tests gracefully skip or verify expected failures in HTTP mode. Additionally, it enhances server readiness checks to handle process exits robustly.
Test reliability and error handling improvements:
ClientToolTests.csto use a newAssertMethodNotFoundAsynchelper, which standardizes assertions for unsupported method errors and properly distinguishes between protocol and HTTP transport modes. [1] [2]AssertMethodNotFoundAsynchelper to handle bothMcpProtocolExceptionandHttpRequestException, depending on the test transport, improving clarity and reducing duplication.Server readiness and process management:
StartHttpServerProcessAndWaitForReadinessAsyncandWaitForServerReadinessAsyncinMcpTestUtilities.csto support cancellation and detect if the server process exits before becoming ready, throwing a clear exception in that case. [1] [2] [3] [4]Live test pipeline configuration:
MCP_TEST_TRANSPORTenvironment variable to'http'in the live test pipeline to ensure tests run in the correct mode.Tool command test robustness in HTTP mode: