`Test-Connection` - Improve Logic and Output #10697

vexx32 · 2019-10-03T21:27:11Z

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

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

vexx32 · 2019-10-04T03:28:06Z

@SteveL-MSFT Samples of output:

Default -Ping Output

-Traceroute Output

-Traceroute -ResolveDestination Output

-MtuSize Output

-IPv6 (with IPv4 address)

-IPv4 (with IPv6 address)

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

vexx32 · 2019-11-11T15:26:52Z

@SteveL-MSFT @iSazonov @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. 😁

SteveL-MSFT · 2019-11-11T16:41:15Z

@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.

vexx32 · 2019-11-11T16:51:45Z

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

#7576

I think this PR addresses that as well 🤔

vexx32 · 2019-11-11T16:54:14Z

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

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

SteveL-MSFT · 2019-11-11T18:45:19Z

...stem.Management.Automation/FormatAndOutput/DefaultFormatters/PowerShellCore_format_ps1xml.cs

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


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

vexx32 · 2019-11-15T17:06:26Z

@PoshChan please retry windows

PoshChan · 2019-11-15T17:06:53Z

@vexx32, successfully started retry of PowerShell-CI-Windows

vexx32 · 2019-11-15T17:27:53Z

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

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


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.

PoshChan · 2019-11-17T03:48:57Z

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

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


@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

@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.

iSazonov · 2019-11-19T03:47:37Z

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

vexx32 · 2019-11-19T04:36:47Z

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! 💖

ghost · 2019-11-21T22:36:11Z

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

Handy links:

Release Notes

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

vexx32 mentioned this pull request Oct 3, 2019

Test-Connection cmdlet displaying unwanted data. #6768

Closed

vexx32 added the WG-Cmdlets-Management cmdlets in the Microsoft.PowerShell.Management module label Oct 3, 2019

🐛 Workaround Ping() class issue

70f6c18

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

PoshChan reviewed Oct 4, 2019

View reviewed changes

src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs Show resolved Hide resolved

vexx32 force-pushed the TestConnection/BetterOutput branch from 6fbafa7 to cf7d523 Compare October 4, 2019 03:39

PoshChan reviewed Oct 4, 2019

View reviewed changes

src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs Show resolved Hide resolved

PoshChan reviewed Oct 4, 2019

View reviewed changes

src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs Show resolved Hide resolved

iSazonov modified the milestones: 7.0.0-preview.5, 7.0.0-preview.6 Oct 24, 2019

vexx32 mentioned this pull request Oct 24, 2019

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

Closed

10 tasks

vexx32 mentioned this pull request Nov 3, 2019

[My bug report]Traceroute Problem #10979

Closed

vexx32 mentioned this pull request Nov 11, 2019

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

Closed

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

iSazonov mentioned this pull request Nov 14, 2019

[My bug report]Ping problem #11062

Closed

vexx32 commented Nov 15, 2019

View reviewed changes

vexx32 added 2 commits November 16, 2019 22:07

Merge branch 'master' into TestConnection/BetterOutput

a45b93d

🎨 fix error in format definitions

91800b4

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.

SteveL-MSFT approved these changes Nov 18, 2019

View reviewed changes

TravisEz13 changed the title ~~Test-Connection - Improve Logic and Output~~ Test-Connection - Improve Logic and Output Nov 18, 2019

TravisEz13 merged commit 4408379 into PowerShell:master Nov 18, 2019

vexx32 deleted the TestConnection/BetterOutput branch November 18, 2019 19:39

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`Test-Connection` - Improve Logic and Output #10697

`Test-Connection` - Improve Logic and Output #10697

vexx32 commented Oct 3, 2019 •

edited

Loading

vexx32 commented Oct 4, 2019 •

edited

Loading

vexx32 commented Nov 11, 2019

SteveL-MSFT commented Nov 11, 2019

vexx32 commented Nov 11, 2019 •

edited

Loading

vexx32 commented Nov 11, 2019

SteveL-MSFT Nov 11, 2019

vexx32 commented Nov 15, 2019

PoshChan commented Nov 15, 2019

vexx32 Nov 15, 2019 •

edited

Loading

vexx32 Nov 17, 2019

PoshChan Nov 17, 2019

vexx32 Nov 17, 2019

TravisEz13 Nov 17, 2019

vexx32 Nov 17, 2019

vexx32 Nov 17, 2019

TravisEz13 Nov 17, 2019

iSazonov commented Nov 19, 2019

vexx32 commented Nov 19, 2019

ghost commented Nov 21, 2019

		.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 #10697

Test-Connection - Improve Logic and Output #10697

Conversation

vexx32 commented Oct 3, 2019 • edited Loading

PR Summary

PR Context

PR Checklist

vexx32 commented Oct 4, 2019 • edited Loading

Default -Ping Output

-Traceroute Output

-Traceroute -ResolveDestination Output

-MtuSize Output

-IPv6 (with IPv4 address)

-IPv4 (with IPv6 address)

vexx32 commented Nov 11, 2019

SteveL-MSFT commented Nov 11, 2019

vexx32 commented Nov 11, 2019 • edited Loading

vexx32 commented Nov 11, 2019

Choose a reason for hiding this comment

vexx32 commented Nov 15, 2019

PoshChan commented Nov 15, 2019

vexx32 Nov 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iSazonov commented Nov 19, 2019

vexx32 commented Nov 19, 2019

ghost commented Nov 21, 2019

`Test-Connection` - Improve Logic and Output #10697

`Test-Connection` - Improve Logic and Output #10697

vexx32 commented Oct 3, 2019 •

edited

Loading

vexx32 commented Oct 4, 2019 •

edited

Loading

vexx32 commented Nov 11, 2019 •

edited

Loading

vexx32 Nov 15, 2019 •

edited

Loading