Skip to content

Centralize test timeout constants to fix CI flakiness#1224

Merged
ericstj merged 4 commits intomainfrom
copilot/add-common-httpclient-timeout
Jan 30, 2026
Merged

Centralize test timeout constants to fix CI flakiness#1224
ericstj merged 4 commits intomainfrom
copilot/add-common-httpclient-timeout

Conversation

Copy link
Contributor

Copilot AI commented Jan 30, 2026 •
edited
Loading

The OAuth.AuthTests.CanAuthenticate_WithoutResourceInWwwAuthenticateHeader_WithPathSuffix test was timing out in CI due to a 10-second HttpClient timeout being insufficient on slow CI machines.

Changes

Test Infrastructure

  • Added TestConstants.HttpClientTimeout (60s) and HttpClientPollingTimeout (2s)
  • Replaced hardcoded HttpClient.Timeout values in:
    • KestrelInMemoryTest.cs (10s → 60s)
    • ServerConformanceTests.cs (2s → 2s, now centralized)
    • EverythingSseServerFixture.cs (2s → 2s, now centralized)
  • Replaced hardcoded WaitForExitAsync timeouts with TestConstants.DefaultTimeout (60s):
    • EverythingSseServerFixture.cs (10s → 60s)
    • StdioServerIntegrationTests.cs (10s → 60s)

All timeout values that represent test infrastructure concerns (not test logic) are now centralized in TestConstants.cs. No production code changes are included in this PR.

Original prompt

This section details on the original issue you should resolve

<issue_title>OAuth.AuthTests.CanAuthenticate_WithoutResourceInWwwAuthenticateHeader_WithPathSuffix timeout in CI</issue_title>
<issue_description>```
ModelContextProtocol.AspNetCore.Tests.OAuth.AuthTests.CanAuthenticate_WithoutResourceInWwwAuthenticateHeader_WithPathSuffix [FAIL]
System.Threading.Tasks.TaskCanceledException : The request was canceled due to the configured HttpClient.Timeout of 10 seconds elapsing.
---- System.TimeoutException : The operation was canceled.
-------- System.Threading.Tasks.TaskCanceledException : The operation was canceled.
------------ System.IO.IOException : The read operation failed, see inner exception.
---------------- System.OperationCanceledException : The operation was canceled.


cc: @ericstj</issue_description>

<agent_instructions>Let's add a common HttpClient timeout for all tests to use. Place it in TestConstants and update any place in test code that sets HttpClient.Timeout to use it.  Let's also use the same in HttpClientTransportOptions.ConnectionTimeout.

Let's also apply the default timeout in TimeoutConstants to calls to WaitForExitAsync.

Try to find any other hardcoded TimeSpans in tests that don't relate to the test's own logic (like polling intervals) and centralize constants for those.</agent_instructions>

## Comments on the Issue (you are @copilot in this section)

<comments>
<comment_new><author>@ericstj</author><body>
Yeah, I've seen this one.  These HTTP client timeouts were different so I didn't change any of those when applying past common timeouts.</body></comment_new>
</comments>

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

@ericstj
Centralize HttpClient and process timeout constants in tests
0c24945 
- Add HttpClientTimeout and HttpClientHealthCheckTimeout constants to TestConstants
- Update all HttpClient.Timeout usages in tests to use centralized constants
- Update all WaitForExitAsync calls to use TestConstants.DefaultTimeout
- Update HttpClientTransportOptions.ConnectionTimeout default from 30s to 60s to match test timeout
- Ensure consistent timeout behavior across tests to prevent CI timeouts

Co-authored-by: ericstj <8918108+ericstj@users.noreply.github.com>
ericstj
ericstj reviewed Jan 30, 2026
View reviewed changes
Copilot AI changed the title [WIP] Add common HttpClient timeout for all tests Centralize test timeout constants to fix CI flakiness Jan 30, 2026
Copilot AI requested a review from ericstj January 30, 2026 18:31
ericstj
ericstj reviewed Jan 30, 2026
View reviewed changes
ericstj
ericstj reviewed Jan 30, 2026
View reviewed changes
Copy link
Contributor

@ericstj ericstj left a comment

Choose a reason for hiding this comment

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

@copilot can you also consider if we need changes in tests\ModelContextProtocol.Tests\Transport\HttpClientTransportTests.cs, and address all other feedback.

@ericstj
Address PR feedback: revert product change and rename constant
6d5b204 
- Revert HttpClientTransportOptions.ConnectionTimeout from 60s back to 30s (no product changes)
- Rename HttpClientHealthCheckTimeout to HttpClientPollingTimeout per feedback
- Update all usages of the renamed constant
- HttpClientTransportTests.cs does not need changes (uses test-specific ConnectionTimeout for mock handlers)

Co-authored-by: ericstj <8918108+ericstj@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Jan 30, 2026

@copilot can you also consider if we need changes in tests\ModelContextProtocol.Tests\Transport\HttpClientTransportTests.cs, and address all other feedback.

Addressed all feedback in commit 6d5b204. Regarding HttpClientTransportTests.cs: This file sets ConnectionTimeout = TimeSpan.FromSeconds(2) on the transport options object, which is test-specific logic (testing with mock handlers that respond immediately). This is different from the infrastructure timeout issues we're addressing, so no changes are needed there.

Copilot AI requested a review from ericstj January 30, 2026 18:39
@ericstj
Copy link
Contributor

ericstj commented Jan 30, 2026

Working on the same issue locally in copilot CLI it also came up with a fix to test service to wait for the service to start.

@ericstj
Copy link
Contributor

ericstj commented Jan 30, 2026

Summary

The issue was a race condition in the OAuth test setup. The TestOAuthServer was started asynchronously in the OAuthTestBase constructor without waiting for it to be fully ready to accept connections. In CI environments with higher load, the OAuth server may take
longer to start, causing tests to timeout when making HTTP requests before the server is ready.

Root Cause

The test infrastructure in OAuthTestBase started the OAuth server with:

    _testOAuthRunTask = TestOAuthServer.RunServerAsync(cancellationToken: TestCts.Token);

This task was never awaited before tests began making requests. Under CI load, if the server wasn't ready when the first OAuth metadata discovery requests were made, the HTTP client would wait until its timeout (10 seconds) was reached.

Fix

- Added ServerStarted property to TestOAuthServer.Program: A TaskCompletionSource that signals when the server has fully started and is ready to accept connections.
- Changed server startup in TestOAuthServer.Program: Replaced app.RunAsync() with app.StartAsync() followed by signaling the TaskCompletionSource, then waiting for cancellation.
- Wait for server readiness in OAuthTestBase.StartMcpServerAsync: Added a wait for TestOAuthServer.ServerStarted before starting the MCP server, ensuring the OAuth server is fully ready before any test requests are made.

@ericstj ericstj marked this pull request as ready for review January 30, 2026 20:08
@ericstj ericstj requested a review from stephentoub January 30, 2026 20:08
stephentoub
stephentoub reviewed Jan 30, 2026
View reviewed changes
stephentoub
stephentoub approved these changes Jan 30, 2026
View reviewed changes
Copy link
Contributor

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Approving so you can merge once you decide if/how to address my pending feedback.

@ericstj ericstj merged commit a83d619 into main Jan 30, 2026
16 of 17 checks passed
@ericstj ericstj deleted the copilot/add-common-httpclient-timeout branch January 30, 2026 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stephentoub stephentoub stephentoub approved these changes

+1 more reviewer

@ericstj ericstj ericstj approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@ericstj ericstj

Copilot code review Copilot

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

OAuth.AuthTests.CanAuthenticate_WithoutResourceInWwwAuthenticateHeader_WithPathSuffix timeout in CI

3 participants

@ericstj @stephentoub