Skip to content

Conversation

@rokonec
Copy link
Member

@rokonec rokonec commented Jun 24, 2025

Fixing: System.Net.Http.Functional.Tests timeouts #115683

Context

Functional tests were starting to intermittently hang.
After analyzing I found that just-running diagnostics tests are causing inserting ActivityRecorder test listener which is failing on some verification by Assert.Same. Some of our test expect that sending request by client part of the client <-> server communication will always succeed and hang on server part accepting incoming request.

Changes made

Since Diagnostics tests use process wide telemetry configuration, which is reverted after test runs, I decided to isolate diagnostic tests which leverage ActivityRecorder from other tests by applying [Collection(nameof(DisableParallelization))] to do its job.

Testing

I was able to consistently reproduce this error by running tests in loop. After changes I am no longer able to repro it.

Note

I noticed that after this changes previously detected intermittent test failures also disappeared.

Copilot AI review requested due to automatic review settings June 24, 2025 09:01
@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jun 24, 2025
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

Isolate DiagnosticsTest by disabling test parallelization to prevent intermittent hangs caused by shared telemetry configuration.

  • Added [Collection(nameof(DisableParallelization))] attribute to the DiagnosticsTest base class.
  • Introduced using Xunit.Sdk; import.

@rokonec rokonec added area-System.Net.Http and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jun 24, 2025
@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 rokonec requested a review from antonfirsov June 24, 2025 09:03
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@MihaZupan
Copy link
Member

#116741 is adding RemoteExecutor to the one test using ActivityRecorder which wasn't already using it. Can you repro the hangs with that change?

@rokonec
Copy link
Member Author

rokonec commented Jun 24, 2025

#116741 is adding RemoteExecutor to the one test using ActivityRecorder which wasn't already using it. Can you repro the hangs with that change?

I am trying to repro it with that PR branch.

I found three tests which might be causing is, that PR address only one, and even if other are not causing it by its internal implementation, leaving it as is would make it too fragile for future changes.

Diagnostic tests seems to properly cleanup after each run, which makes me believe we can achieve proper isolation by disabling concurrent running. I prefer this to RemoteExecutor because it is much easier to debug and maybe faster to run.

@rokonec rokonec marked this pull request as draft June 24, 2025 11:16
@rokonec
Copy link
Member Author

rokonec commented Jun 24, 2025

Converting to draft for now as it seems that #116741 will address it.

@rokonec
Copy link
Member Author

rokonec commented Jun 27, 2025

Seems to be solved by other PR

@rokonec rokonec closed this Jun 27, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Jul 28, 2025
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.

2 participants