Add -WhatIf switch to Start-Process cmdlet by sarithsutha · Pull Request #4735 · PowerShell/PowerShell · GitHub

Add -WhatIf switch to Start-Process cmdlet

Sarith Sutha (@sarithsutha),
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

Perhaps add a test that would normally perform an action and ensure that -WhatIf prevented the action?
For example, have a Start-Process command to write a string to a file and then test to ensure the file was not written to with -WhatIf is supplied.

Maybe also a test that using the -PassThru switch, no object gets returned (which is also a good additional test that the command was not executed)?

It "returns null when using -WhatIf switch with `PassThru`" {
     { Start-Process $pingCommand -ArgumentList $pingParam -PassThru -WhatIf } | Should Be $null
}

Thanks for your suggestion Christoph Bergmeister (@bergmeister) . I've added the test that you described.

… from being performed.

Thanks for your suggestion Mark Kraus (@markekraus) . I've added that test.

I think the test does not need - next do the check.

I'm sorry, I did not get which - were you referring to.

I meant next It block.

Maybe:

$pingOutput | Should Not Exist

We re-use ProcessNameForConfirmation in Stop-Process and Debug-Process so I believe we can use the same in Start-process.

The original request was to output the exact command being executed. By re-using the ProcessNameForConfirmation, the format of the output will be changed slightly, the arguments would appear in the parentheses. i.e. C:\WINDOWS\system32\PING.EXE (-n 2 localhost) instead of C:\WINDOWS\system32\PING.EXE -n 2 localhost.
If that is acceptable, I'm happy to make the change.

Thanks for clarify.
Closed.

We can remove next It and do the check here.

I believe -ErrorAction Stop is required or it will never actually throw.

I agree.

Thanks Mark Kraus (@markekraus) for pointing this out. Not sure how I missed it.

AppVeyor failed on other tests so the PR LGTM with one minor comment..

Thanks Ilya (@iSazonov) for reviewing and approving my changes. The minor comment is now incorporated.

LGTM

Dongbo Wang (@daxian-dbw) Could you please review and approve?

The resource string id ProcessStartInfo should be changed to StartProcessTarget.

Dongbo Wang (@daxian-dbw) Renamed the resource string as suggested.

Sarith Sutha (@sarithsutha) Thanks!

Sarith Sutha (@sarithsutha) Thanks for your contribution!

* Add -WhatIf switch to Start-Process cmdlet * Add test for the -WhatIf switch * Added a test to ensure that using a whatif switch prevents the action from being performed. * Incorporated code review comments * merged two tests into one as suggested * Included the error action at the end of the cmdlet * Remove extra space before `StringUtil` and new line * Renamed the resource string id ProcessStartInfo to StartProcessTarget

Add -WhatIf switch to Start-Process cmdlet

3fa6423

Sarith Sutha (sarithsutha) requested review from Aditya Patwardhan (adityapatwardhan) and Dongbo Wang (daxian-dbw) as code owners September 1, 2017 15:20

Microsoft Contribution License Agreements (msftclas) added the cla-already-signed label Sep 1, 2017

Mark Kraus (markekraus) reviewed Sep 1, 2017

View reviewed changes

Added a test to ensure that using a whatif switch prevents the action…

326a924

… from being performed.

Ilya (iSazonov) reviewed Sep 4, 2017

View reviewed changes

Incorporated code review comments

b2333fb

Ilya (iSazonov) reviewed Sep 5, 2017

View reviewed changes

merged two tests into one as suggested

f73b26b

Mark Kraus (markekraus) reviewed Sep 5, 2017

View reviewed changes

Ilya (iSazonov) approved these changes Sep 5, 2017

View reviewed changes

Included the error action at the end of the cmdlet

97d10e2

Mark Kraus (markekraus) approved these changes Sep 5, 2017

View reviewed changes

Dongbo Wang (daxian-dbw) assigned Ilya (iSazonov) Sep 5, 2017

Remove extra space before StringUtil and new line

85a31ab

Dongbo Wang (daxian-dbw) requested changes Sep 6, 2017

View reviewed changes

Renamed the resource string id ProcessStartInfo to StartProcessTarget

ff9556c

Dongbo Wang (daxian-dbw) approved these changes Sep 7, 2017

View reviewed changes

Ilya (iSazonov) merged commit f4b075c into PowerShell:master Sep 8, 2017

Sarith Sutha (sarithsutha) deleted the start-process-enhancements branch September 8, 2017 13:42

+                  It "Should be able to use the whatif switch without performing the actual action" {
+                      $pingOutput = Join-Path $TestDrive "pingOutput.txt"
+                      Start-Process -Wait $pingCommand -ArgumentList $pingParam -RedirectStandardOutput $pingOutput -WhatIf
+                      Test-Path $pingOutput | Should Be $false

                 </data>
+                <data name="ProcessStartInfo" xml:space="preserve">
+                  <value>{0} {1}</value>
+                </data>

+                  It "Should be able to use the -WhatIf switch without performing the actual action" {
                       $pingOutput = Join-Path $TestDrive "pingOutput.txt"
-                      Start-Process -Wait $pingCommand -ArgumentList $pingParam -RedirectStandardOutput $pingOutput -WhatIf
+                      { Start-Process -Wait $pingCommand -ArgumentList $pingParam -RedirectStandardOutput $pingOutput -WhatIf} | Should Not Throw

+                  It "Should be able to use the whatif switch without error" {
+                      { Start-Process $pingCommand -ArgumentList $pingParam -WhatIf } | Should Not Throw
+                  }

                               startInfo.WindowStyle = _windowstyle;
                           }
+                          string targetMessage = StringUtil.Format(ProcessResources.ProcessStartInfo, startInfo.FileName, startInfo.Arguments.Trim());

Add -WhatIf switch to Start-Process cmdlet by sarithsutha · Pull Request #4735 · PowerShell/PowerShell · GitHub

Uh oh!

Add -WhatIf switch to Start-Process cmdlet - #4735

Add -WhatIf switch to Start-Process cmdlet#4735Ilya (iSazonov) merged 7 commits intoPowerShell:masterPowerShell/PowerShell:masterfrom sarithsutha:start-process-enhancementssarithsutha/PowerShell:start-process-enhancementsCopy head branch name to clipboard

Conversation

Sarith Sutha (sarithsutha) commented Sep 1, 2017

Uh oh!

Microsoft Contribution License Agreements (msftclas) commented Sep 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Christoph Bergmeister (bergmeister) Sep 3, 2017 • 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!

Sarith Sutha (sarithsutha) commented Sep 1, 2017

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) Sep 5, 2017 • 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!

Ilya (iSazonov) left a comment • 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!

Sarith Sutha (sarithsutha) commented Sep 5, 2017

Uh oh!

Mark Kraus (markekraus) left a comment

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) commented Sep 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dongbo Wang (daxian-dbw) left a comment

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) commented Sep 8, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Add -WhatIf switch to Start-Process cmdlet#4735
Ilya (iSazonov) merged 7 commits into
PowerShell:masterfrom
sarithsutha:start-process-enhancements

Christoph Bergmeister (bergmeister) Sep 3, 2017 •
edited
Loading

Ilya (iSazonov) Sep 5, 2017 •
edited
Loading

Ilya (iSazonov) left a comment •
edited
Loading