Skip to content

Commit 6a02c5a

Browse files
ericstjericstj
authored
Add testing guidelines, update copilot instructions for tests (#1507)
1 parent 2e7209c commit 6a02c5a

File tree

2 files changed

+97
-16
lines changed

2 files changed

+97
-16
lines changed

‎.github/copilot-instructions.md‎

Lines changed: 33 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -85,22 +85,39 @@ The SDK consists of three main packages:
8585
- Test servers in `tests/ModelContextProtocol.Test*Server/` for integration scenarios
8686
- Filter manual tests with `[Trait("Execution", "Manual")]` - these require external dependencies
8787

88-
### Test Infrastructure and Helpers
89-
- **LoggedTest**: Base class for tests that need logging output captured to xUnit test output
90-
- Provides `ILoggerFactory` and `ITestOutputHelper` for test logging
91-
- Use when debugging or when tests need to verify log output
92-
- **TestServerTransport**: In-memory transport for testing client-server interactions without network I/O
93-
- **MockLoggerProvider**: For capturing and asserting on log messages
94-
- **XunitLoggerProvider**: Routes `ILogger` output to xUnit's `ITestOutputHelper`
95-
- **KestrelInMemoryTransport** (AspNetCore.Tests): In-memory Kestrel connection for HTTP transport testing without network stack
96-
97-
### Test Best Practices
98-
- Inherit from `LoggedTest` for tests needing logging infrastructure
99-
- Use `TestServerTransport` for in-memory client-server testing
100-
- Mock external dependencies (filesystem, HTTP clients) rather than calling real services
101-
- Use `CancellationTokenSource` with timeouts to prevent hanging tests
102-
- Dispose resources properly (servers, clients, transports) using `IDisposable` or `await using`
103-
- Run tests with: `dotnet test --filter '(Execution!=Manual)'`
88+
### Test Base Classes
89+
- **`LoggedTest`**: Base class that wires up `ILoggerFactory` with `XunitLoggerProvider` (test output) and `MockLoggerProvider` (log assertions). Inherit from this for any test needing logging.
90+
- **`ClientServerTestBase`**: Sets up in-memory client/server pair via `Pipe`. Override `ConfigureServices` to register tools/prompts/resources, then call `CreateMcpClientForServer()`. Handles async disposal automatically.
91+
- **`KestrelInMemoryTest`** (AspNetCore tests): Hosts ASP.NET Core with in-memory transport — no ports needed.
92+
- **`TestServerTransport`**: In-memory mock transport for testing client logic without a real server.
93+
94+
### Transport Selection in Tests
95+
- **Never use `WithStdioServerTransport()` in unit tests.** It reads from the test host's stdin, which cannot be closed, permanently leaking a thread pool thread per test.
96+
- For DI-only tests: `WithStreamServerTransport(Stream.Null, Stream.Null)`
97+
- For client/server interaction: inherit `ClientServerTestBase`
98+
- For client-only logic: use `TestServerTransport`
99+
- For HTTP/SSE: inherit `KestrelInMemoryTest`
100+
- For process lifecycle tests: `StdioClientTransport` (only when testing actual process behavior)
101+
102+
### Resource Management
103+
- **Always `await using` the `ServiceProvider`** when MCP server services are registered — `McpServerImpl` only implements `IAsyncDisposable`. Synchronous `using` throws at runtime.
104+
- **Always dispose clients and servers** — use `await using var client = ...`
105+
- **Use `TestContext.Current.CancellationToken`** for async MCP calls so xUnit can cancel on timeout.
106+
107+
### Timeouts
108+
- **Always use `TestConstants.DefaultTimeout`** (60s) instead of hardcoded values. CI machines are slower than dev workstations.
109+
- For HTTP polling operations use `TestConstants.HttpClientPollingTimeout` (2s).
110+
111+
### Synchronization
112+
- **Never use `Task.Delay` for synchronization.** Use `TaskCompletionSource`, `SemaphoreSlim`, or `Channel` so tests don't depend on timing.
113+
114+
### Background Logging
115+
- `ITestOutputHelper.WriteLine` throws after the test method returns. Background threads (process event handlers, async continuations) can outlive the test, causing unhandled exceptions that crash the test host.
116+
- Route logging through `LoggedTest.LoggerFactory` — `XunitLoggerProvider` already catches post-test exceptions.
117+
- If calling `ITestOutputHelper` directly from an event handler, wrap in try/catch for `InvalidOperationException`.
118+
119+
### Parallelism
120+
- Tests run in parallel by default. Apply `[Collection(nameof(DisableParallelization))]` to test classes that touch global state (e.g., `ActivitySource` listeners).
104121

105122
## Build and Development
106123

‎CONTRIBUTING.md‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,70 @@ dotnet test tests/ModelContextProtocol.Tests/
6565

6666
Tools like Visual Studio, JetBrains Rider, and VS Code also provide integrated test runners that can be used to run and debug individual tests.
6767

68+
### Writing Tests
69+
70+
The test projects include shared infrastructure in `tests/Common/Utils/` that most tests build on. Familiarize yourself with these helpers before writing new tests.
71+
72+
#### Test base classes
73+
74+
- **`LoggedTest`** — Base class that wires up `ILoggerFactory` with both `XunitLoggerProvider` (routes to test output) and `MockLoggerProvider` (captures logs for assertions). Inherit from this for any test that needs logging.
75+
- **`ClientServerTestBase`** — Sets up an in-memory client/server pair connected via `Pipe` with proper async disposal. Override `ConfigureServices` to register tools, prompts, and resources, then call `CreateMcpClientForServer()` to get a connected client.
76+
- **`KestrelInMemoryTest`** (ASP.NET Core tests) — Hosts an ASP.NET Core server with in-memory transport so HTTP/SSE tests run without allocating ports.
77+
78+
#### Choosing a transport
79+
80+
| Scenario | Transport | Why |
81+
|---|---|---|
82+
| Unit tests that only need DI | `WithStreamServerTransport(Stream.Null, Stream.Null)` | No threads blocked, no process spawned |
83+
| Client/server interaction tests | `ClientServerTestBase` (uses `Pipe`) | Full bidirectional MCP, in-process |
84+
| Client-only logic | `TestServerTransport` | In-memory mock that auto-responds to standard MCP requests |
85+
| HTTP/SSE integration | `KestrelInMemoryTest` | Real HTTP stack, no network |
86+
| External process tests | `StdioClientTransport` | Only when testing actual process lifecycle |
87+
88+
> **Do not** use `WithStdioServerTransport()` in unit tests. The stdio server transport reads from the test host process's standard input, which the test does not own and cannot close. This means the transport's background read loop can never terminate, permanently leaking a thread pool thread per test. Use `WithStreamServerTransport(Stream.Null, Stream.Null)` for tests that only need the DI container.
89+
90+
#### Resource management
91+
92+
- **Always `await using` the `ServiceProvider`** when MCP server services are registered — `McpServerImpl` only implements `IAsyncDisposable`, not `IDisposable`. A synchronous `using` will throw at runtime, and skipping disposal leaks transports and background threads.
93+
- **Use `TestContext.Current.CancellationToken`** when calling async MCP methods so that xUnit can cancel the test on timeout rather than hanging.
94+
- **Dispose clients and servers** explicitly. Prefer `await using var client = ...` over relying on finalizers. `ClientServerTestBase` handles this if you inherit from it.
95+
96+
#### Timeouts
97+
98+
Use `TestConstants.DefaultTimeout` (60 seconds) rather than hardcoded values. CI machines are often slower than developer workstations, and short timeouts cause flaky failures.
99+
100+
```csharp
101+
// Good
102+
using var cts = CancellationTokenSource.CreateLinkedTokenSource(TestContext.Current.CancellationToken);
103+
cts.CancelAfter(TestConstants.DefaultTimeout);
104+
await client.CallToolAsync("my-tool", args, cts.Token);
105+
106+
// Bad — too short for CI
107+
cts.CancelAfter(TimeSpan.FromSeconds(5));
108+
```
109+
110+
#### Synchronization
111+
112+
Avoid `Task.Delay` for synchronization. Use explicit signaling primitives (`TaskCompletionSource`, `SemaphoreSlim`, `Channel`) so tests don't depend on timing. If a producer/consumer test writes events for a streaming reader, use a `TaskCompletionSource` to confirm the reader is active before writing.
113+
114+
#### Background logging
115+
116+
`ITestOutputHelper.WriteLine` throws after the test method returns. Background threads (process event handlers, async continuations) can outlive the test. This manifests as unhandled exceptions that crash the test host. Two mitigations:
117+
118+
1. **`XunitLoggerProvider`** already catches these exceptions. Route logging through `LoggedTest.LoggerFactory` rather than calling `ITestOutputHelper` directly from callbacks.
119+
2. **If you must call `ITestOutputHelper` from an event handler**, wrap it in a try/catch:
120+
```csharp
121+
process.ErrorDataReceived += (s, e) =>
122+
{
123+
try { testOutputHelper.WriteLine(e.Data); }
124+
catch (InvalidOperationException) { }
125+
};
126+
```
127+
128+
#### Parallelism
129+
130+
Tests run in parallel by default. If a test class touches global state (e.g., `ActivitySource` listeners in diagnostics tests), apply `[Collection(nameof(DisableParallelization))]` to run it sequentially.
131+
68132
### Building the Documentation
69133

70134
This project uses [DocFX](https://dotnet.github.io/docfx/) to generate its conceptual and reference documentation.

0 commit comments

Comments
 (0)