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

sarithsutha · 2017-09-01T15:20:34Z

Add -WhatIf switch to Start-Process cmdlet

msftclas · 2017-09-01T15:20:37Z

@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

markekraus · 2017-09-01T19:32:42Z

test/powershell/Modules/Microsoft.PowerShell.Management/Start-Process.Tests.ps1

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


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 @bergmeister . I've added the test that you described.

… from being performed.

sarithsutha · 2017-09-01T20:07:02Z

Thanks for your suggestion @markekraus . I've added that test.

iSazonov · 2017-09-04T10:56:30Z

test/powershell/Modules/Microsoft.PowerShell.Management/Start-Process.Tests.ps1

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


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.

iSazonov · 2017-09-04T10:58:26Z

test/powershell/Modules/Microsoft.PowerShell.Management/Start-Process.Tests.ps1

+    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


Maybe:

$pingOutput | Should Not Exist

iSazonov · 2017-09-04T11:06:04Z

src/Microsoft.PowerShell.Commands.Management/resources/ProcessResources.resx

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


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.

iSazonov · 2017-09-05T14:24:03Z

test/powershell/Modules/Microsoft.PowerShell.Management/Start-Process.Tests.ps1

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


We can remove next It and do the check here.

markekraus · 2017-09-05T17:31:54Z

test/powershell/Modules/Microsoft.PowerShell.Management/Start-Process.Tests.ps1

+    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


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

Thanks @markekraus for pointing this out. Not sure how I missed it.

iSazonov

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

sarithsutha · 2017-09-05T18:16:13Z

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

markekraus

LGTM

iSazonov · 2017-09-06T04:34:58Z

@daxian-dbw Could you please review and approve?

daxian-dbw · 2017-09-06T20:53:10Z

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

                startInfo.WindowStyle = _windowstyle;
            }

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


The resource string id ProcessStartInfo should be changed to StartProcessTarget.

@daxian-dbw Renamed the resource string as suggested.

daxian-dbw

@sarithsutha Thanks!

iSazonov · 2017-09-08T05:19:05Z

@sarithsutha Thanks for your contribution!

Add -WhatIf switch to Start-Process cmdlet

3fa6423

sarithsutha requested review from adityapatwardhan and daxian-dbw as code owners September 1, 2017 15:20

msftclas added the cla-already-signed label Sep 1, 2017

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.

iSazonov reviewed Sep 4, 2017

View reviewed changes

Incorporated code review comments

b2333fb

iSazonov reviewed Sep 5, 2017

View reviewed changes

merged two tests into one as suggested

f73b26b

markekraus reviewed Sep 5, 2017

View reviewed changes

iSazonov approved these changes Sep 5, 2017

View reviewed changes

Included the error action at the end of the cmdlet

97d10e2

markekraus approved these changes Sep 5, 2017

View reviewed changes

daxian-dbw assigned iSazonov Sep 5, 2017

Remove extra space before StringUtil and new line

85a31ab

daxian-dbw requested changes Sep 6, 2017

View reviewed changes

Renamed the resource string id ProcessStartInfo to StartProcessTarget

ff9556c

daxian-dbw approved these changes Sep 7, 2017

View reviewed changes

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

sarithsutha deleted the start-process-enhancements branch September 8, 2017 13:42

Conversation

sarithsutha commented Sep 1, 2017

Uh oh!

msftclas commented Sep 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

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!

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!

sarithsutha commented Sep 5, 2017

Uh oh!

markekraus left a comment

Choose a reason for hiding this comment

Uh oh!

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!

daxian-dbw left a comment

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Sep 8, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bergmeister Sep 3, 2017 •

edited

Loading

iSazonov Sep 5, 2017 •

edited

Loading

iSazonov left a comment •

edited

Loading