Harden CancelSendPingAsync tests #116588
Merged
Harden CancelSendPingAsync tests #116588
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
needs-area-label
antonfirsov
added
area-System.Net
and removed
needs-area-label
|
Tagging subscribers to this area: @dotnet/ncl |
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Pull Request Overview
This PR separates the existing CancelSendPingAsync test into two focused scenarios—hostname and IPAddress variants—and hardens the IPAddress test by pinging unreachable addresses with retry logic to reduce CI flakiness.
- Split
CancelSendPingAsyncintoCancelSendPingAsync_HostNameandCancelSendPingAsync_IPAddressmethods. - Updated the IPAddress test to iterate over multiple unreachable IPs and retry if the network immediately replies.
- Removed combined host/IP logic and streamlined cancellation calls.
Comments suppressed due to low confidence (1)
src/libraries/System.Net.Ping/tests/FunctionalTests/PingTest.cs:715
- [nitpick] The comment on the
OuterLoopattribute refers to an "external host", but this test targets unreachable IPs. Updating the comment to reflect dependency on unreachable-address behavior would improve clarity.
[OuterLoop] // Depends on external host and assumption that successful ping takes long enough for cancellation to go through first
src/libraries/System.Net.Ping/tests/FunctionalTests/PingTest.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <[email protected]>
…onfirsov/runtime into harden-CancelSendPingAsync-1
This comment was marked as outdated.
This comment was marked as outdated.
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Outerloop failures are unrelated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #113831.
Separate
SendPingAsync(hostname)andSendPingAsync(IPAddress)test implementations:SendPingAsync(hostname): we didn't see issues with this variant in the CI. It effectively tests if the resolution ofwww.microsoft.comis cancelled as part of theSendPingAsynccall. Keep the existing logic in a separate method.SendPingAsync(IPAddress): occasionally, pinging the valid IP likely resulted in replies before the cancellation could kick in on Helix Mac machines. Instead, let's ping an unreachable IP to avoid this. Since that can also sometimes result to an ICMP reply instead of a timeout, use the same trick as in HardenPing_TimedOut_*tests #116500, repeating the test with other hosts if it happens.