`Test-Connection` - Improve Logic and Output by vexx32 · Pull Request #10697 · PowerShell/PowerShell · GitHub

PR Summary

Refactor Test-Connection to provide unit-based output with clearer formatting and more useful traceroute timing information.

Uses SendAsync() to send pings and adds event handler to permit Ctrl+C to cancel the cmdlet operation mid-ping (great for giving up on a long timeout).
Refactor output; both regular -Ping and -TraceRoute actions output one object per received ping, formatted to accommodate as much information as possible.
Refactor -Traceroute methodology to use 4 pings instead of 3:
- 1-3 initial "discovery" pings with low TTL to the final destination to find the router at that hop point (not displayed in output). First ping to come back with TtlExpired is used to target the rest of the pings.
  - On Unix, TtlExpired is never reported, so we skip this step completely and just target all pings at the final destination.
- 3 followup pings to that router directly so we get proper latency and status information from the router (all rendered in output).
- This fixes an outstanding issue where traceroutes could not provide latency information on intermediate hops.
Slight refactor for MtuSize detection to allow it to work correctly on Unix.
Use new class types for all output in order to better provide easy access to displayed information.
Update ValidateSet attribute on -MaxHops to reflect underlying API limitations (0 is not a valid argument for PingOptions' TTL)
Fix long-standing issue with -Traceroute where -MaxHops is a valid input parameter, but is completely ignored. New behaviour is to write an error if the destination cannot be reached within the set value for -MaxHops, or return $false if also specifying -Quiet.
Fixed an issue where -IPv4 and -IPv6 were completely ignored if you supplied a valid address from the other family. Resolution is to check the AddressFamily, and if it doesn't match the provided switch, pull the host entry and find an address matching the requested family to use.
- Added some additional IPv6 tests. After running all the IPv6 tests in CI (including those originally marked -Pending) I think it best we leave them disabled. I've verified the features work in Windows 10 and Ubuntu under WSL, but Azures CI apparently doesn't have great IPv6 support, so it's a very mixed bag on what comes back for those tests. We can reenable them in future if there are improvements or alterations to CI config that might improve their reliability there.
Renamed switches:
- -Continues => -Repeat (Alias applied to maintain compatibility)
- -MtuSizeDetect => -MtuSize (Alias applied here also)

PR Context

RFC: PowerShell/PowerShell-RFC#172

Resolves #9235

Resolves #7685

PR Checklist

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
This PR is ready to merge and is not Work in Progress.
- If the PR is work in progress, please add the prefix WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
Breaking changes
- None
- OR
- Experimental feature(s) needed
  - Experimental feature name(s):
User-facing changes
- Not Applicable
- OR
- Documentation needed
  - Issue filed: Doc changes to Test-Connection MicrosoftDocs/PowerShell-Docs#4987
Testing - New and feature
- N/A or can only be tested interactively
- OR
- Make sure you've added a new test if existing tests do not effectively test the code changed
Tooling
- I have considered the user experience from a tooling perspective and don't believe tooling will be impacted.
- OR
- I have considered the user experience from a tooling perspective and enumerated concerns in the summary. This may include:
  - Impact on PowerShell Editor Services which is used in the PowerShell extension for VSCode (which runs in a different PS Host).
  - Impact on Completions (both in the console and in editors) - one of PowerShell's most powerful features.
  - Impact on PSScriptAnalyzer (which provides linting & formatting in the editor extensions).
  - Impact on EditorSyntax (which provides syntax highlighting with in VSCode, GitHub, and many other editors).

Rather than instantiate a new Ping() every time, we can store it in a readonly field and just call Send() as needed.

This uses the SendAsync() method with a manual reset shim, allowing us to cancel the ping if needed. For example, in the case of StopProcessing() being called, the cmdlet can properly process the request and halt processing, throwing the correct errors as needed.

All output is provided as soon as the PingReply is received, using a new class to format the output (PingStatus). Also added a FormatView for PingStatus.

🎨 Update formatview definitions to include traceroutes ♻️ move formatviewdefintions to proper location in file

♻️ Small amount of cleanup

Errors out when trying to SendPingAsync() too quickly in succession

Steve Lee (@SteveL-MSFT) Samples of output:

Default -Ping Output

-Traceroute Output

-Traceroute -ResolveDestination Output

-MtuSize Output

-IPv6 (with IPv4 address)

-IPv4 (with IPv6 address)

Steve Lee (@SteveL-MSFT) Ilya (@iSazonov) Travis Plunk (@TravisEz13) I checked https://github.com/dotnet/corefx/issues/28934 recently and saw they pushed it back to 5.0 for now.

Can we make a judgement call on the way forward here? This implementation, while doing more work than it should need to, is at least (in my opinion) more usable and functional on every platform than the existing implementation, and makes use of the most effective parts of the API surface currently available.

In my opinion, having this in PS7 to have a more user-friendly cmdlet would be better than waiting until .NET 5 for the API to be fixed. 🙂

I'm more than happy to do the later rewrite when the .NET Core team (finally) fix their Ping APIs. 😁

Rain Sallow (/u/ta11ow) (@vexx32) I don't think we should hold this waiting on .NET (should open a new issue linked to corefx, I can tag it for vNext). I'll spend some time reviewing this today.

Went back through ping / test-connection issues and found this one:

#7576

I think this PR addresses that as well 🤔

Steve Lee (@SteveL-MSFT) looks like we've been tracking this in #4240; I think we can continue to track this there. 🙂

Since this is formatting, we can make changes in the future based on user feedback

PoshChan-Bot (@PoshChan) please retry windows

Rain Sallow (/u/ta11ow) (@vexx32), successfully started retry of PowerShell-CI-Windows

Interesting. Querying the Reply.Address in the latest build I got from CI appears to be... broken.

 PS>  $a.Reply.Address

OverloadDefinitions
-------------------
System.Object&, System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e Address(int )

I'll have to make sure I rebase this PR and see if this is resolved in a later .NET core preview. Querying the address in different ways does work, but something is a bit screwy with the auto-property. Gonna look into this probably this evening, should hopefully not be a complex fix. 🤞

(This is a bit of an oddity because this actually causes it to be hidden in the formatter, which is just super odd.)

FIgured it out. Pushing a fix.

Rain Sallow (/u/ta11ow) (@vexx32), your last commit had 1 failures in PowerShell-CI-windows
Test-Connection.Ping.Force IPv4 with explicit PingOptions

Expected $true, but got $false.
at <ScriptBlock>, D:\a\1\s\test\powershell\Modules\Microsoft.PowerShell.Management\Test-Connection.Tests.ps1: line 103
103:                 $result1.Reply.Options.DontFragment | Should -BeTrue

Steve Lee (@SteveL-MSFT) I have confirmed with a manual test that this regression comes from .NET Core itself -- a manual use of the Ping.Send() or SendAsync() exhibits this behaviour where a DontFragment setting is ignored (or at least not returned as set from the ping reply).

This appears to be a new regression since the 3.0 release.

This API is... getting more fragile. 😕

Marking this test as pending until this gets fixed.

Did someone file an issue?

I added a note on the existing tracking issue in coreFX for the Ping APIs.

One of the coreFX folx asked if I could set breakpoints and check what's going on. I may give it a stab, but I'm not really sure what I'm doing there tbh. CoreFX is a very large project, and I'm not familiar with how to work with it in a debugging context, or even build it at all.

Should I create an additional tracking issue here as well? We already have a tracking issue for the APIs used by this cmdlet in general I think I can add a note to.

I'm good as long as it's tracked somehow.

PingOptions are not returned from .NET Core api as expected. Test marked as pending for now.

Should we document anything (the check box is set)?

Yeah, we changed quite a bit in terms of the shape of the output in pretty much all the cases here.

Doc issue is MicrosoftDocs/PowerShell-Docs#4987 - I'll link it in the PR description as well. Thanks for the reminder! 💖

🎉v7.0.0-preview.6 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

Rain Sallow (/u/ta11ow) (vexx32) added 22 commits August 24, 2019 00:48

♻️ Rename parameter set constants

6f5774f

📝 Rename parameter sets

897ec6e

🎨 Whitespace style fixes

f02c95a

🎨 Code analysis style pass

a9db3c9

♻️ Simplify parameter sets

8954c9e

♻️ Address CodeFactor nits

1ec1d03

♻️ Address Codacy nits

6c32cac

♻️ Simplify type names

140efb2

♻️ Use single Ping() object for all Send()

6ca1f58

Rather than instantiate a new Ping() every time, we can store it in a readonly field and just call Send() as needed.

✨ Add IDisposable implementation

046451f

🔧 Move information stream output to verbose

c99199b

♻️ Rename method a little more simply

febe0da

✨ Revamp standard Ping output

cb50486

All output is provided as soon as the PingReply is received, using a new class to format the output (PingStatus). Also added a FormatView for PingStatus.

✨ Improve output for traceroute mode

f55c2aa

🎨 Update formatview definitions to include traceroutes ♻️ move formatviewdefintions to proper location in file

✨ Fix -MtuSizeDetect switch output

2ee6a8f

♻️ Use WriteVerbose instead of Progress bar

5805bb8

🎨 📝 Whitespace and comment changes

fbc189c

♻️ Refactor parameters

c13f08c

♻️ Organise private fields into regions

67c407d

♻️ Small amount of cleanup

messy rebase changes goin' in (#4 )

8622cac

Finish merge

5315a5d

Rain Sallow (/u/ta11ow) (vexx32) mentioned this pull request Oct 3, 2019

Test-Connection cmdlet displaying unwanted data. #6768

Closed

Rain Sallow (/u/ta11ow) (vexx32) added the Area-Cmdlets-Management label Oct 3, 2019

🐛 Workaround Ping() class issue

70f6c18

Errors out when trying to SendPingAsync() too quickly in succession

PoshChan-Bot (PoshChan) reviewed Oct 4, 2019

View reviewed changes

Comment thread src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs

Rain Sallow (/u/ta11ow) (vexx32) force-pushed the TestConnection/BetterOutput branch from 6fbafa7 to cf7d523 Compare October 4, 2019 03:39

PoshChan-Bot (PoshChan) reviewed Oct 4, 2019

View reviewed changes

Comment thread src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs Outdated

PoshChan-Bot (PoshChan) reviewed Oct 4, 2019

View reviewed changes

Comment thread src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs Outdated

Ilya (iSazonov) added this to the 7.0.0-preview.6 milestone Oct 24, 2019

Rain Sallow (/u/ta11ow) (vexx32) mentioned this pull request Oct 24, 2019

Doc changes to Test-Connection MicrosoftDocs/PowerShell-Docs#4987

Closed

10 tasks

Rain Sallow (/u/ta11ow) (vexx32) mentioned this pull request Nov 3, 2019

[My bug report]Traceroute Problem #10979

Closed

Rain Sallow (/u/ta11ow) (vexx32) mentioned this pull request Nov 11, 2019

Limitations .Net Core to migrate Test-Connection cmdlet #4240

Closed

Steve Lee (SteveL-MSFT) requested changes Nov 11, 2019

View reviewed changes

ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Nov 11, 2019

Apply suggestions from code review

3564740

Ilya (iSazonov) mentioned this pull request Nov 14, 2019

[My bug report]Ping problem #11062

Closed

Rain Sallow (/u/ta11ow) (vexx32) commented Nov 15, 2019

View reviewed changes

Rain Sallow (/u/ta11ow) (vexx32) added 2 commits November 16, 2019 22:07

Merge branch 'master' into TestConnection/BetterOutput

a45b93d

🎨 fix error in format definitions

91800b4

PoshChan-Bot (PoshChan) reviewed Nov 17, 2019

View reviewed changes

✅ Mark test as pending

f912e68

PingOptions are not returned from .NET Core api as expected. Test marked as pending for now.

Steve Lee (SteveL-MSFT) approved these changes Nov 18, 2019

View reviewed changes

Travis Plunk (TravisEz13) changed the title Test-Connection - Improve Logic and Output Test-Connection - Improve Logic and Output Nov 18, 2019

Travis Plunk (TravisEz13) merged commit 4408379 into PowerShell:master Nov 18, 2019

Rain Sallow (/u/ta11ow) (vexx32) deleted the TestConnection/BetterOutput branch November 18, 2019 19:39

Rain Sallow (/u/ta11ow) (vexx32) mentioned this pull request Nov 18, 2019

Test-Connect/Ping Tests forcing IPv4 is unreliable - assumes GetHostEntry address list is in a specific order #7576

Closed

ghost mentioned this pull request Nov 21, 2019

Test-Connection on PowerShell 6.1 RC-1, Does not give correct output? #7685

Closed

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj/PowerShell that referenced this pull request Aug 6, 2026

Test-Connection - Improve Logic and Output (PowerShell#10697 )

90f39a5

+                          /// <summary>
+                          /// Gets the target address of the ping.
+                          /// </summary>
+                          public IPAddress? Address { get => Reply.Status == IPStatus.Success ? Reply.Address : null; }

		.AddHeader(Alignment.Right, label: "Latency(ms)", width: 7)
		.AddHeader(Alignment.Right, label: "BufferSize(B)", width: 10)

		@@ -1,8 +1,12 @@
		// Copyright (c) Microsoft Corporation. All rights reserved.

`Test-Connection` - Improve Logic and Output by vexx32 · Pull Request #10697 · PowerShell/PowerShell · GitHub

Uh oh!

Test-Connection - Improve Logic and Output - #10697

Test-Connection - Improve Logic and Output#10697Travis Plunk (TravisEz13) merged 65 commits intoPowerShell:masterPowerShell/PowerShell:masterfrom vexx32:TestConnection/BetterOutputvexx32/PowerShell:TestConnection/BetterOutputCopy head branch name to clipboard

Conversation

Rain Sallow (/u/ta11ow) (vexx32) commented Oct 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Context

PR Checklist

Uh oh!

Rain Sallow (/u/ta11ow) (vexx32) commented Oct 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Default -Ping Output

-Traceroute Output

-Traceroute -ResolveDestination Output

-MtuSize Output

-IPv6 (with IPv4 address)

-IPv4 (with IPv6 address)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Rain Sallow (/u/ta11ow) (vexx32) commented Nov 11, 2019

Uh oh!

Steve Lee (SteveL-MSFT) commented Nov 11, 2019

Uh oh!

Rain Sallow (/u/ta11ow) (vexx32) commented Nov 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rain Sallow (/u/ta11ow) (vexx32) commented Nov 11, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rain Sallow (/u/ta11ow) (vexx32) commented Nov 15, 2019

Uh oh!

PoshChan-Bot (PoshChan) commented Nov 15, 2019

Uh oh!

Rain Sallow (/u/ta11ow) (vexx32) Nov 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) commented Nov 19, 2019

Uh oh!

Rain Sallow (/u/ta11ow) (vexx32) commented Nov 19, 2019

Uh oh!

Deleted user (ghost) commented Nov 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Test-Connection - Improve Logic and Output#10697
Travis Plunk (TravisEz13) merged 65 commits into
PowerShell:masterfrom
vexx32:TestConnection/BetterOutput

Rain Sallow (/u/ta11ow) (vexx32) commented Oct 3, 2019 •
edited
Loading

Rain Sallow (/u/ta11ow) (vexx32) commented Oct 4, 2019 •
edited
Loading

Rain Sallow (/u/ta11ow) (vexx32) commented Nov 11, 2019 •
edited
Loading

Rain Sallow (/u/ta11ow) (vexx32) Nov 15, 2019 •
edited
Loading