Refactor Test-Connection #10044

vexx32 · 2019-07-01T23:10:00Z

PR Summary

Refactor the Test-Connection cmdlet according to PowerShell/PowerShell-RFC#172

PR Context

Resolves #6768

NOTE: Currently this changes the pattern from Ping.Send() to Ping.SendAsync() with a manual event handler. This is due to a bug that was only recently resolved (see https://github.com/dotnet/corefx/issues/38770#issuecomment-507788270)

We can return to the simpler Ping.Send() pattern once we hit .NET Core 3-preview8 per the comment in that issue. 🙂

TODO:

Cleanup verbose messages
Add tests

Additional suggestions:

Add sequence numbers or timestamps to the standard PingStatus items so it's easy to see the order they were received in. Test-Connection Changes PowerShell-RFC#172 (comment) - @bobfrankly
Add the ability to ping all IP addresses in a given CIDR range (DDoS concerns maybe?) - @romero126

PR Checklist

Simplify some variable declarations, fix up some uber long lines, etc.

& some misc cleanup. Implementing IDisposable allows us to put the Ping() instances used everywhere into a single field and dispose it whenever the command is.

Kinda in the name aight?

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

iSazonov

@vexx32 Please move all style changes in 1,2,3 new simple PRs.

Also I think it makes no sense create a workaround with async if we get Core Preview8 soon.

iSazonov · 2019-07-03T10:05:27Z

test/powershell/Modules/Microsoft.PowerShell.Management/Test-Connection.Tests.ps1

-            { Test-Connection $targetName -BufferSize 0 }     | Should Not Throw
-            { Test-Connection $targetName -BufferSize -1 }    | Should -Throw -ErrorId "ParameterArgumentValidationError,Microsoft.PowerShell.Commands.TestConnectionCommand"
-            { Test-Connection $targetName -BufferSize 65501 } | Should -Throw -ErrorId "ParameterArgumentValidationError,Microsoft.PowerShell.Commands.TestConnectionCommand"
+            { Test-Connection $targetName -BufferSize 0 } | Should -Not Throw


Suggested change

{ Test-Connection $targetName -BufferSize 0 } | Should -Not Throw

{ Test-Connection $targetName -BufferSize 0 } | Should -Not -Throw

vexx32 · 2019-07-03T10:57:31Z

Not sure it even makes sense to split style changes to be honest as this changes / rewrites a heck of a lot that I would cover in a style PR anyway.

We're still blocked on preview6 so I'm leaving the temp fix in for now so we can trust test results. When we get preview8 it isn't difficult to switch the pattern back.

iSazonov · 2019-07-03T11:25:15Z

Have pity!😭 We have open about 50 PRs...

vexx32 · 2019-07-03T13:19:29Z

I know. But is one of these better, or four for the same thing? 😕

If you'd really rather extra style PRs I can probably look to trim some of that out, but in a lot of ways the style changes are kind of tied into the rest of the changes. Let me know what you'd prefer! 😊

iSazonov · 2019-07-03T13:44:50Z

@vexx32 You will spend some time for temporary PRs but save a lot of time of reviewers.
It's incredible that someone can view more than 400-500 changes at a time.
After PR exeeds 100 changes, any style change significantly slows down the code review and begins to annoy.
While the PR draft you could make temporary PRs. I see many renames.
Also you could split functional changes too, ex. separate ping, traceroute and etc. to different PRs.
As for async, we get Preview7 in 5 weeks. I suppose we have nowhere to hurry.

vexx32 · 2019-07-03T13:46:31Z

Yeah, I guess that makes sense.

I'll look to do a style pass and see how I can split it out from there. Thanks! 💖
I may keep working in here till I see what I mucked up in the tests but as soon as I have this sorted I'll start splitting. 🙂

…TestConnectionRefactor

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

Bug with native exceptions was fixed in .NET Core 3-preview7 Should be safe to use synchronous ping.

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

vexx32 · 2019-10-04T17:55:33Z

Closing this as this has been superceded by #10697

vexx32 added 30 commits July 1, 2019 18:50

🎨 Apply Roslyn analyzer fixes

9d29809

🎨 Whitespace style fixes

8603a4b

🎨 style pass for odds and ends 8000

ea04a0b

Simplify some variable declarations, fix up some uber long lines, etc.

✨ Add new PingStatus class

7a70349

✨ implement IDisposable

89cda88

& some misc cleanup. Implementing IDisposable allows us to put the Ping() instances used everywhere into a single field and dispose it whenever the command is.

✨ Update traceroute functionality

4c14c17

🚧 Fiddling with information displays.

86c5566

🚧 Add Status property to PingStatus

04c57cd

Kinda in the name aight?

✨ tidy up traceroutes

23aba08

🚧 Flesh out PingStatus class

712afbe

♻️ remove unused PSHost tag

cab8764

📝 Update xml doc tags for TraceStatus

7897be5

🚧 Simplify param sets & clean

30ca329

🔧 Fix up parameter sets

31fbdda

🚧 Remove unnecessary method call

3be2450

♻️ Remove unused fields

44d84f3

🔧 Fix invalid cast

e06a544

🔧 have hostname take proper value in trace

8e59c05

✨ assess status for trace specifically

527c7aa

♻️ Shuffle properties around a bit

1017306

🎨 add initial formatview for PingStatus

1b72fef

🎨 Add grouping to PingStatus format

ada3228

🎨 remove grouped prop from columns

bfe0073

🎨 Remove stray header

b666998

♻️ Refactor TraceStatus to singletons

ee93272

🎨 Add PingMtuSize formatview

307cb69

🎨 ♻️ Update TraceStatus formatter

e1dbbd9

♻️ finish flattening trace output

1921ddf

🚧 Tidy up classes

43d2ffe

PoshChan reviewed Jul 3, 2019

View reviewed changes

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

✅ Update tests & formatting

3d723da

PoshChan reviewed Jul 3, 2019

View reviewed changes

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

PoshChan reviewed Jul 3, 2019

View reviewed changes

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

PoshChan reviewed Jul 3, 2019

View reviewed changes

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

iSazonov reviewed Jul 3, 2019

View reviewed changes

vexx32 mentioned this pull request Aug 16, 2019

Test-Connection -Quiet not working #10370

Closed

vexx32 added 2 commits August 23, 2019 12:13

Merge branch 'master' into TestConnectionRefactor

307d838

Merge branch 'master' of git://github.com/PowerShell/PowerShell into …

42cce00

…TestConnectionRefactor

PoshChan reviewed Aug 23, 2019

View reviewed changes

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

PoshChan reviewed Aug 23, 2019

View reviewed changes

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

PoshChan reviewed Aug 23, 2019

View reviewed changes

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

PoshChan reviewed Aug 23, 2019

View reviewed changes

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

♻️ Remove async ping framework

33a7cf8

Bug with native exceptions was fixed in .NET Core 3-preview7 Should be safe to use synchronous ping.

PoshChan reviewed Aug 23, 2019

View reviewed changes

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

PoshChan reviewed Aug 23, 2019

View reviewed changes

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

PoshChan reviewed Aug 23, 2019

View reviewed changes

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

♻️ simplify attribute declarations

236b981

PoshChan reviewed Aug 23, 2019

View reviewed changes

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

PoshChan reviewed Aug 23, 2019

View reviewed changes

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

PoshChan reviewed Aug 23, 2019

View reviewed changes

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

vexx32 mentioned this pull request Aug 24, 2019

Cleanup TestConnectionCommand #10439

Merged

14 tasks

vexx32 mentioned this pull request Sep 3, 2019

Test-Connection - Remove informational output, consolidate Ping usage #10478

Merged

14 tasks

vexx32 closed this Oct 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Refactor Test-Connection #10044

Refactor Test-Connection #10044

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	{ Test-Connection $targetName -BufferSize 0 } \| Should -Not Throw
	{ Test-Connection $targetName -BufferSize 0 } \| Should -Not -Throw

Refactor Test-Connection #10044

Refactor Test-Connection #10044

Uh oh!

Conversation

Uh oh!

PR Summary

PR Context

TODO:

Additional suggestions:

PR Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants