test(mcp): assert smoke harness connectivity deterministically - #1832
Open
Aaronontheweb wants to merge 4 commits into
Open
test(mcp): assert smoke harness connectivity deterministically#1832Aaronontheweb wants to merge 4 commits into
Aaronontheweb wants to merge 4 commits into
Conversation
StartAsync awaits the whole connect attempt, so by the time it returns the manager has either published tools to the registry or recorded a failure status with the underlying error message. Asserting Connected via GetServerStatuses() turns an intermittent Windows CI connect failure (previously a bare Assert.NotNull null on the tool lookup in SmokeMcpServerHttpHeaderTests) into a failure that names the real error, instead of silently swallowing it.
Wire the McpClientManager logger to xunit test output in the HTTP smoke harness. ReportConnectionFailure logs the full exception, but NullLogger discarded it — the tests only ever saw the generic 'Failed to reach MCP server' status message. On the intermittent Windows connect flake, this now prints the underlying exception to the CI log so the root cause can be fixed precisely instead of guessed.
Aaronontheweb
added
mcp
Aaronontheweb
enabled auto-merge (squash)
August 9, 2026 13:14
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the Windows CI flake in
SmokeMcpServerHttpHeaderTests. The failing test asserted tool registration with a bare null check that hid the underlying connect failure.McpClientManager.StartAsyncawaits the whole connect attempt. When it returns, the manager has either published tools to the registry or recorded a failure status with the real error message. The tests never checked that status. An intermittent Windows connect failure surfaced asAssert.NotNull() Failure: Value is nullat the tool lookup instead of the actual error. The same test passed on Ubuntu and macOS.Changes
McpSmokeHarness.AssertConnected, which asserts the server reachedConnectedstate and fails with the manager's stored error message.StartAsyncin all four header smoke tests.The assertion is deterministic. It reads a completion signal that already exists — no polling, no timeout adjustments. When the server fails to connect, the test now reports the connect error instead of a null.
Verified locally: all 4
SmokeMcpServerHttpHeaderTestspass; the Daemon.Tests project builds with 0 warnings.