Refactor Test-Connection by vexx32 · Pull Request #10044 · PowerShell/PowerShell · GitHub

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

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:
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

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?

Rain Sallow (/u/ta11ow) (@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.

Suggested change

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

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.

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

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

Rain Sallow (/u/ta11ow) (@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.

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

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

Closing this as this has been superceded by #10697

Rain Sallow (/u/ta11ow) (vexx32) added 30 commits July 1, 2019 18:50

🎨 Apply Roslyn analyzer fixes

9d29809

🎨 Whitespace style fixes

8603a4b

🎨 style pass for odds and ends

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

🚧 Update MtuSize and Ping methods

02821e1

♻️ 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-Bot (PoshChan) reviewed Jul 3, 2019

View reviewed changes

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

✅ Update tests & formatting

3d723da

PoshChan-Bot (PoshChan) reviewed Jul 3, 2019

View reviewed changes

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

PoshChan-Bot (PoshChan) reviewed Jul 3, 2019

View reviewed changes

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

PoshChan-Bot (PoshChan) reviewed Jul 3, 2019

View reviewed changes

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

Ilya (iSazonov) reviewed Jul 3, 2019

View reviewed changes

Rain Sallow (/u/ta11ow) (vexx32) mentioned this pull request Aug 16, 2019

Test-Connection -Quiet not working #10370

Closed

Rain Sallow (/u/ta11ow) (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-Bot (PoshChan) reviewed Aug 23, 2019

View reviewed changes

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

PoshChan-Bot (PoshChan) reviewed Aug 23, 2019

View reviewed changes

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

PoshChan-Bot (PoshChan) reviewed Aug 23, 2019

View reviewed changes

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

PoshChan-Bot (PoshChan) reviewed Aug 23, 2019

View reviewed changes

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

♻️ Remove async ping framework

33a7cf8

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

PoshChan-Bot (PoshChan) reviewed Aug 23, 2019

View reviewed changes

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

PoshChan-Bot (PoshChan) reviewed Aug 23, 2019

View reviewed changes

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

PoshChan-Bot (PoshChan) reviewed Aug 23, 2019

View reviewed changes

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

♻️ simplify attribute declarations

236b981

PoshChan-Bot (PoshChan) reviewed Aug 23, 2019

View reviewed changes

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

PoshChan-Bot (PoshChan) reviewed Aug 23, 2019

View reviewed changes

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

PoshChan-Bot (PoshChan) reviewed Aug 23, 2019

View reviewed changes

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

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

Cleanup TestConnectionCommand #10439

Merged

14 tasks

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

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

Merged

14 tasks

Rain Sallow (/u/ta11ow) (vexx32) closed this Oct 4, 2019

-                          { 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

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

Refactor Test-Connection by vexx32 · Pull Request #10044 · PowerShell/PowerShell · GitHub

Uh oh!

Refactor Test-Connection - #10044

Refactor Test-Connection#10044Rain Sallow (/u/ta11ow) (vexx32) wants to merge 65 commits intoPowerShell:masterPowerShell/PowerShell:masterfrom vexx32:TestConnectionRefactorvexx32/PowerShell:TestConnectionRefactorCopy head branch name to clipboard

Conversation

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

Uh oh!

PR Summary

PR Context

TODO:

Additional suggestions:

PR Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Ilya (iSazonov) left a comment

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) Jul 3, 2019

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

Ilya (iSazonov) commented Jul 3, 2019

Uh oh!

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

Uh oh!

Uh oh!

Ilya (iSazonov) commented Jul 3, 2019

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Refactor Test-Connection#10044
Rain Sallow (/u/ta11ow) (vexx32) wants to merge 65 commits into
PowerShell:masterfrom
vexx32:TestConnectionRefactor

Rain Sallow (/u/ta11ow) (vexx32) commented Jul 1, 2019 •
edited
Loading

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

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

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