-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Fix test Http3_WaitForConnection_RecordedWhenWaitingForStream #116741
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
Fix test Http3_WaitForConnection_RecordedWhenWaitingForStream #116741
Conversation
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.
Pull Request Overview
This PR fixes an issue with a test that uses ActivityRecorder without proper process isolation, preventing interference from parallel tests. The key changes include:
- Marking CreateHttp3LoopbackServer as static in HttpClientHandlerTestBase.SocketsHttpHandler.cs.
- Updating DiagnosticsTests.cs to require RemoteExecutor support and wrapping the test logic within RemoteExecutor.Invoke.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTestBase.SocketsHttpHandler.cs | Made CreateHttp3LoopbackServer static for improved clarity and test isolation. |
| src/libraries/System.Net.Http/tests/FunctionalTests/DiagnosticsTests.cs | Updated the test condition to use RemoteExecutor and refactored test logic into a static local function for isolation. |
Comments suppressed due to low confidence (2)
src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTestBase.SocketsHttpHandler.cs:66
- Marking CreateHttp3LoopbackServer as static is appropriate since it doesn't depend on instance state. Please verify that no existing code depends on an instance method signature.
protected static Http3LoopbackServer CreateHttp3LoopbackServer(Http3Options options = default)
src/libraries/System.Net.Http/tests/FunctionalTests/DiagnosticsTests.cs:1594
- Using RemoteExecutor to wrap the test ensures isolation from other parallel runs. Confirm that the test environment supports RemoteExecutor for consistent behavior.
[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
|
Tagging subscribers to this area: @dotnet/ncl |
|
It is same root cause as in #116955. However, during investigation I have noticed another two tests which are using ActivityRecorder: Maybe we should move those to RemoteExecutor as well. |
|
@rokonec those 2 do utilze I merged this now, if we have further ideas we can file them in separate PR-s. Also note that #115683 has been reported before the merge of #114083, so I wouldn't exclude the option that there may exist alternative reasons for the timeouts, unrelated to |
#114083 incorrectly added a test that uses
ActivityRecorderwithout process isolation viaRemoteExecutor.This may lead to the registered
ActivityListenercallbacks firing from unrelated tests running in parallel. I started seeing frequent local failures because of this, the stack trace in #116279 (comment) also seems to be related.Fixes #116279.