-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Modernize DiagnosticsHandler tests #53870
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
Conversation
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsEffectively all tests here were in OuterLoop/disabled, even those that were not hitting any servers. This PR:
Fixes #23167 (re-enables the tests disabled against it)
|
src/libraries/System.Net.Http/tests/FunctionalTests/DiagnosticsTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/tests/FunctionalTests/DiagnosticsTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs
Outdated
Show resolved
Hide resolved
geoffkizer
left a comment
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.
Generally looks good -- nice to see this get modernized. A few small issues or things to consider.
SemaphoreSlim => TCS, Guid => nosuchhost.invalid, Http20 => Http2
|
Hello @MihaZupan! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
Effectively all tests here were in OuterLoop/disabled, even those that were not hitting any servers.
This PR:
SpinWait.SpinUntilwithSemaphoreSlimwhere needed)Hierarchicalformat wasn't being tested)Fixes #23167 (re-enables the tests disabled against it)