Skip to content

[tests] Improve LinkSdk.DllImportTest.PingSend to try to ping multiple hosts. #25147

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.

Sign up for GitHub

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

Merged
rolfbjarne merged 1 commit into from
Apr 15, 2026
Merged

[tests] Improve LinkSdk.DllImportTest.PingSend to try to ping multiple hosts. #25147

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions tests/linker/link sdk/DllImportTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@
//

using System.Net.NetworkInformation;
using System.Linq;
using System.Threading.Tasks;

#if !__MACOS__
using UIKit;
#endif
Expand Down Expand Up @@ -78,10 +81,17 @@ public void Sqlite3 ()
[Test]
public void PingSend ()
{
var p = new Ping ();
// support was implemented with https://github.com/dotnet/runtime/pull/52240
var reply = p.Send ("localhost");
Assert.That (reply.Status, Is.EqualTo (IPStatus.Success), "Pong");
var hosts = new string [] {
"localhost",
"www.microsoft.com",
"microsoft.com",
Comment on lines +86 to +88
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using external hosts like www.microsoft.com/microsoft.com introduces a dependency on outbound network + ICMP availability (often blocked in CI, captive networks, or sandboxed environments), which can make this test flaky. If the goal is to validate Ping support, prefer loopback IPs (e.g., 127.0.0.1 and ::1) or other local-only targets that don’t require DNS/external connectivity.

Suggested change
"localhost",
"www.microsoft.com",
"microsoft.com",
"127.0.0.1",
"::1",

Copilot uses AI. Check for mistakes.
};
var timeout = TimeSpan.FromSeconds (10);
var tasks = hosts.Select (host => (new Ping ()).SendPingAsync (host, timeout)).ToArray ();
Assert.IsTrue (Task.WaitAll (tasks, timeout.Add (TimeSpan.FromSeconds (10))), "One or more of the ping requests timed out.");
var results = tasks.Select (task => task.Result.Status).ToArray ();
Assert.That (results, Has.Some.EqualTo (IPStatus.Success), "Pong any host");
Comment on lines +91 to +94
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Task.WaitAll (tasks, ...) waits for all pings to complete, and will also throw if any SendPingAsync task faults (e.g., DNS failure/ICMP blocked). This contradicts the intent to “pass if any ping succeeds” and can make the test fail even when one host replies. Consider waiting for completion in a way that tolerates per-host failures/timeouts (e.g., observe each task’s status/exception and only require at least one IPStatus.Success).

Suggested change
var tasks = hosts.Select (host => (new Ping ()).SendPingAsync (host, timeout)).ToArray ();
Assert.IsTrue (Task.WaitAll (tasks, timeout.Add (TimeSpan.FromSeconds (10))), "One or more of the ping requests timed out.");
var results = tasks.Select (task => task.Result.Status).ToArray ();
Assert.That (results, Has.Some.EqualTo (IPStatus.Success), "Pong any host");
var tasks = hosts.Select (host => (new Ping ()).SendPingAsync (host, timeout)).ToList ();
var overallTimeout = timeout.Add (TimeSpan.FromSeconds (10));
var deadline = System.DateTime.UtcNow + overallTimeout;
while (tasks.Count > 0) {
var remaining = deadline - System.DateTime.UtcNow;
if (remaining <= TimeSpan.Zero)
break;
var completed = Task.WhenAny (tasks).GetAwaiter ().GetResult ();
if (!completed.Wait (remaining))
break;
tasks.Remove (completed);
if (completed.Status != TaskStatus.RanToCompletion)
continue;
if (completed.Result.Status == IPStatus.Success)
return;
}
Assert.Fail ("Pong any host");

Copilot uses AI. Check for mistakes.
Comment on lines +91 to +94
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping implements IDisposable, but the new code creates multiple instances and never disposes them. Please ensure the Ping instances are disposed after the ping tasks finish (even on failures/timeouts) to avoid leaking native resources during the test run.

Suggested change
var tasks = hosts.Select (host => (new Ping ()).SendPingAsync (host, timeout)).ToArray ();
Assert.IsTrue (Task.WaitAll (tasks, timeout.Add (TimeSpan.FromSeconds (10))), "One or more of the ping requests timed out.");
var results = tasks.Select (task => task.Result.Status).ToArray ();
Assert.That (results, Has.Some.EqualTo (IPStatus.Success), "Pong any host");
var pings = hosts.Select (host => new Ping ()).ToArray ();
var tasks = pings.Zip (hosts, (ping, host) => ping.SendPingAsync (host, timeout)).ToArray ();
try {
Assert.IsTrue (Task.WaitAll (tasks, timeout.Add (TimeSpan.FromSeconds (10))), "One or more of the ping requests timed out.");
var results = tasks.Select (task => task.Result.Status).ToArray ();
Assert.That (results, Has.Some.EqualTo (IPStatus.Success), "Pong any host");
} finally {
foreach (var ping in pings)
ping.Dispose ();
}

Copilot uses AI. Check for mistakes.
}
}
}
Loading