Skip to content

Fix for #4520 '-ArgumentList should accept @() or $null' #6597

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
adityapatwardhan merged 7 commits into from
Jun 22, 2018
Merged

Fix for #4520 '-ArgumentList should accept @() or $null' #6597

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d85c2a5
Fix for #4520 '-ArgumentList should accept @() or $null'
Apr 9, 2018
94983c5
Fixed null and @() empty list tests to not both test null
Apr 11, 2018
b0c3bed
[release] Abort changes and trying over.
Apr 25, 2018
436cc81
[Feature] Fixed test issue on non-Windows; added code to suppress dis…
Apr 25, 2018
01603a5
Fixing up problems caused by a bad merge
Apr 26, 2018
45f0ccf
Merge branch 'master' into brucepay_StartProcessArgs
Jun 1, 2018
324d772
[Feature]
adityapatwardhan Jun 21, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions src/Microsoft.PowerShell.Commands.Management/commands/management/Process.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,6 @@

namespace Microsoft.PowerShell.Commands
{
// 2004/12/17-JonN ProcessNameGlobAttribute was deeply wrong.
// For example, if you pass in a single Process, it will match
// all processes with the same name.
// I have removed the globbing code.

#region ProcessBaseCommand
/// <summary>
/// This class implements the base for process commands
Expand Down Expand Up @@ -461,7 +456,6 @@ public sealed class GetProcessCommand : ProcessBaseCommand
/// <summary>
/// Has the list of process names on which to this command will work
/// </summary>
// [ProcessNameGlobAttribute]
[Parameter(Position = 0, ParameterSetName = NameParameterSet, ValueFromPipelineByPropertyName = true)]
[Parameter(Position = 0, ParameterSetName = NameWithUserNameParameterSet, ValueFromPipelineByPropertyName = true)]
[Alias("ProcessName")]
Expand Down Expand Up @@ -1062,7 +1056,6 @@ public sealed class StopProcessCommand : ProcessBaseCommand
/// <summary>
/// Has the list of process names on which to this command will work
/// </summary>
// [ProcessNameGlobAttribute]
[Parameter(
ParameterSetName = "Name",
Mandatory = true,
Expand Down Expand Up @@ -1618,7 +1611,6 @@ public sealed class StartProcessCommand : PSCmdlet, IDisposable
/// </summary>
[Parameter(Position = 1)]
[Alias("Args")]
[ValidateNotNullOrEmpty]
[SuppressMessage("Microsoft.Performance", "CA1819:PropertiesShouldNotReturnArrays")]
public string[] ArgumentList { get; set; }

Expand Down
44 changes: 33 additions & 11 deletions test/powershell/Modules/Microsoft.PowerShell.Management/Start-Process.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ Describe "Start-Process" -Tags @("Feature") {
$isNanoServer = [System.Management.Automation.Platform]::IsNanoServer
$isIot = [System.Management.Automation.Platform]::IsIoT
$isFullWin = $IsWindows -and !$isNanoServer -and !$isIot
$extraArgs = @{}
if ($isFullWin) {
$extraArgs.WindowStyle = "Hidden"
}

$pingCommand = (Get-Command -CommandType Application ping)[0].Definition
$pingDirectory = Split-Path $pingCommand -Parent
Expand All @@ -23,65 +27,65 @@ Describe "Start-Process" -Tags @("Feature") {
# This has been fixed on Linux, but not on macOS

It "Should process arguments without error" {
$process = Start-Process ping -ArgumentList $pingParam -PassThru -RedirectStandardOutput "$TESTDRIVE/output"
$process = Start-Process ping -ArgumentList $pingParam -PassThru -RedirectStandardOutput "$TESTDRIVE/output" @extraArgs

$process.Length | Should -Be 1
$process.Id | Should -BeGreaterThan 1
# $process.ProcessName | Should Be "ping"
}

It "Should work correctly when used with full path name" {
$process = Start-Process $pingCommand -ArgumentList $pingParam -PassThru -RedirectStandardOutput "$TESTDRIVE/output"
$process = Start-Process $pingCommand -ArgumentList $pingParam -PassThru -RedirectStandardOutput "$TESTDRIVE/output" @extraArgs

$process.Length | Should -Be 1
$process.Id | Should -BeGreaterThan 1
# $process.ProcessName | Should Be "ping"
}

It "Should invoke correct path when used with FilePath argument" {
$process = Start-Process -FilePath $pingCommand -ArgumentList $pingParam -PassThru -RedirectStandardOutput "$TESTDRIVE/output"
$process = Start-Process -FilePath $pingCommand -ArgumentList $pingParam -PassThru -RedirectStandardOutput "$TESTDRIVE/output" @extraArgs

$process.Length | Should -Be 1
$process.Id | Should -BeGreaterThan 1
# $process.ProcessName | Should Be "ping"
}

It "Should invoke correct path when used with Path alias argument" {
$process = Start-Process -Path $pingCommand -ArgumentList $pingParam -PassThru -RedirectStandardOutput "$TESTDRIVE/output"
$process = Start-Process -Path $pingCommand -ArgumentList $pingParam -PassThru -RedirectStandardOutput "$TESTDRIVE/output" @extraArgs

$process.Length | Should -Be 1
$process.Id | Should -BeGreaterThan 1
}

It "Should wait for command completion if used with Wait argument" {
$process = Start-Process ping -ArgumentList $pingParam -Wait -PassThru -RedirectStandardOutput "$TESTDRIVE/output"
$process = Start-Process ping -ArgumentList $pingParam -Wait -PassThru -RedirectStandardOutput "$TESTDRIVE/output" @extraArgs
}

It "Should work correctly with WorkingDirectory argument" {
$process = Start-Process ping -WorkingDirectory $pingDirectory -ArgumentList $pingParam -PassThru -RedirectStandardOutput "$TESTDRIVE/output"
$process = Start-Process ping -WorkingDirectory $pingDirectory -ArgumentList $pingParam -PassThru -RedirectStandardOutput "$TESTDRIVE/output" @extraArgs

$process.Length | Should -Be 1
$process.Id | Should -BeGreaterThan 1
# $process.ProcessName | Should Be "ping"
}

It "Should handle stderr redirection without error" {
$process = Start-Process ping -ArgumentList $pingParam -PassThru -RedirectStandardError $tempFile -RedirectStandardOutput "$TESTDRIVE/output"
$process = Start-Process ping -ArgumentList $pingParam -PassThru -RedirectStandardError $tempFile -RedirectStandardOutput "$TESTDRIVE/output" @extraArgs

$process.Length | Should -Be 1
$process.Id | Should -BeGreaterThan 1
# $process.ProcessName | Should Be "ping"
}

It "Should handle stdout redirection without error" {
$process = Start-Process ping -ArgumentList $pingParam -Wait -RedirectStandardOutput $tempFile
$process = Start-Process ping -ArgumentList $pingParam -Wait -RedirectStandardOutput $tempFile @extraArgs
$dirEntry = get-childitem $tempFile
$dirEntry.Length | Should -BeGreaterThan 0
}

# Marking this test 'pending' to unblock daily builds. Filed issue : https://github.com/PowerShell/PowerShell/issues/2396
It "Should handle stdin redirection without error" -Pending {
$process = Start-Process sort -Wait -RedirectStandardOutput $tempFile -RedirectStandardInput $assetsFile
$process = Start-Process sort -Wait -RedirectStandardOutput $tempFile -RedirectStandardInput $assetsFile @extraArgs
$dirEntry = get-childitem $tempFile
$dirEntry.Length | Should -BeGreaterThan 0
}
Expand Down Expand Up @@ -116,13 +120,31 @@ Describe "Start-Process" -Tags @("Feature") {

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 -ErrorAction Stop } | Should -Not -Throw
{ Start-Process -Wait $pingCommand -ArgumentList $pingParam -RedirectStandardOutput $pingOutput -WhatIf -ErrorAction Stop @extraArgs} | Should -Not -Throw
$pingOutput | Should -Not -Exist
}

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

It 'Should run without errors when -ArgumentList is $null' {
$process = Start-Process $pingCommand -ArgumentList $null -PassThru @extraArgs
$process.Length | Should -Be 1
$process.Id | Should -BeGreaterThan 1
Copy link
Member

@adityapatwardhan adityapatwardhan Jun 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this check required?

Copy link
Collaborator Author

@BrucePay BrucePay Jun 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are basic sanity tests to make sure the process was created and is running (has an ID).

Copy link
Collaborator

@iSazonov iSazonov Jun 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the process is not started the $process is $null.

}

It "Should run without errors when -ArgumentList is @()" {
$process = Start-Process $pingCommand -ArgumentList @() -PassThru @extraArgs
$process.Length | Should -Be 1
$process.Id | Should -BeGreaterThan 1
Copy link
Member

@adityapatwardhan adityapatwardhan Jun 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

Copy link
Collaborator Author

@BrucePay BrucePay Jun 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are basic sanity tests to make sure the process was created and is running (has an ID).

}

It "Should run without errors when -ArgumentList is ''" {
Copy link
Collaborator

@iSazonov iSazonov Apr 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use -TestCases and replace last three tests with one.

Copy link
Collaborator Author

@BrucePay BrucePay Apr 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather do this in a separate check-in. This whole file needs serious clean up e.g. there are tests that depend on notepad.

$process = Start-Process $pingCommand -ArgumentList '' -PassThru @extraArgs
$process.Length | Should -Be 1
$process.Id | Should -BeGreaterThan 1
Copy link
Member

@adityapatwardhan adityapatwardhan Jun 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

Copy link
Collaborator Author

@BrucePay BrucePay Jun 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are basic sanity tests to make sure the process was created and is running (has an ID).

}
}

Describe "Start-Process tests requiring admin" -Tags "Feature","RequireAdminOnWindows" {
Expand Down