Skip to content

Conversation

@antonfirsov
Copy link
Contributor

#114083 incorrectly added a test that uses ActivityRecorder without process isolation via RemoteExecutor.

This may lead to the registered ActivityListener callbacks 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.

@antonfirsov antonfirsov requested review from a team and Copilot June 17, 2025 13:40
Copy link
Contributor

Copilot AI left a 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))]

@dotnet-policy-service
Copy link
Contributor

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

@rokonec
Copy link
Member

rokonec commented Jun 24, 2025

It is same root cause as in #116955.

However, during investigation I have noticed another two tests which are using ActivityRecorder:
SendAsync_ConnectionFailure_RecordsActivitiesWithCorrectErrorInfo
SendAsync_Success_ConnectionSetupActivityGraphRecorded

Maybe we should move those to RemoteExecutor as well.

@antonfirsov antonfirsov merged commit aa70ad6 into dotnet:main Jun 24, 2025
80 of 86 checks passed
@antonfirsov
Copy link
Contributor Author

antonfirsov commented Jun 24, 2025

@rokonec those 2 do utilze RemoteExecutor already for isolation. Do you have more info on failures related to those tests?

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 ActivityRecorder. I would keep #115683 open for a while and monitor if the failures still keep happening.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

System.Net.Http.Functional.Tests: Exception type was not an exact match

4 participants