Skip to content

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

Merged
Aditya Patwardhan (adityapatwardhan) merged 7 commits into
PowerShell:masterfrom
BrucePay:brucepay_StartProcessArgs
Jun 22, 2018
Merged

Fix for #4520 '-ArgumentList should accept @() or $null'#6597
Aditya Patwardhan (adityapatwardhan) merged 7 commits into
PowerShell:masterfrom
BrucePay:brucepay_StartProcessArgs

Conversation

@BrucePay

@BrucePay Bruce Payette (BrucePay) commented Apr 9, 2018 •
edited by adityapatwardhan
Loading

Copy link
Copy Markdown
Collaborator

Fix #4520

Just removed the [ValidateIsNotNulOrEmpty] attribute from the ArgumentList parameter. (Also removed some obsolete comments).

PR Summary

PR Checklist

iSazonov
Ilya (iSazonov) reviewed Apr 9, 2018
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Management/Start-Process.Tests.ps1 Outdated

@iSazonov Ilya (iSazonov) Apr 9, 2018 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should it be -ArgumentList "" ?
And should we tests -ArgumentList @() too?

@markekraus

Mark Kraus (markekraus) commented Apr 12, 2018

Copy link
Copy Markdown
Contributor

Bruce Payette (@BrucePay) can you please push an empty commit with [feature]? The tests in this PR are gated by the feature flag.

@SteveL-MSFT

Steve Lee (SteveL-MSFT) commented Apr 12, 2018

Copy link
Copy Markdown
Member

Bruce Payette (@BrucePay) I believe GitHub won't auto-close issues in the title, so I put Fix #4520 in the PR description which works.

For Mark Kraus (@markekraus)' request, you can just do git commit --amend and update the description where the first line is [feature] which will kick off the feature tests in CI

iSazonov
Ilya (iSazonov) reviewed Apr 12, 2018
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Management/Start-Process.Tests.ps1 Outdated

@iSazonov Ilya (iSazonov) Apr 12, 2018

Copy link
Copy Markdown
Collaborator

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.

@BrucePay Bruce Payette (BrucePay) Apr 25, 2018

Copy link
Copy Markdown
Collaborator Author

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.

@SteveL-MSFT

Steve Lee (SteveL-MSFT) commented Apr 25, 2018

Copy link
Copy Markdown
Member

Bruce Payette (@BrucePay) looks like you accidentally pulled in other commits into this PR. Find me tomorrow in the office to get this cleaned up.

@BrucePay

Bruce Payette (BrucePay) commented Apr 25, 2018

Copy link
Copy Markdown
Collaborator Author

Steve Lee (@SteveL-MSFT) Not sure how it happened bu oh my yes :-( I will definitely find you.

@BrucePay
Bruce Payette (BrucePay) force-pushed the brucepay_StartProcessArgs branch 3 times, most recently from d2da162 to 761748a Compare April 26, 2018 18:51
adityapatwardhan
Aditya Patwardhan (adityapatwardhan) requested changes Jun 20, 2018
View reviewed changes

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please submit the last commit with [Feature] in the commit message to execute the tests in the PR.

git commit --allow-empty
git push

Comment thread test/powershell/Modules/Microsoft.PowerShell.Management/Start-Process.Tests.ps1
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

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Jun 20, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this check required?

@BrucePay Bruce Payette (BrucePay) Jun 20, 2018

Copy link
Copy Markdown
Collaborator Author

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).

@iSazonov Ilya (iSazonov) Jun 21, 2018

Copy link
Copy Markdown
Collaborator

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.

Comment thread test/powershell/Modules/Microsoft.PowerShell.Management/Start-Process.Tests.ps1
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

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Jun 20, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same as above.

@BrucePay Bruce Payette (BrucePay) Jun 20, 2018

Copy link
Copy Markdown
Collaborator Author

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).

Comment thread test/powershell/Modules/Microsoft.PowerShell.Management/Start-Process.Tests.ps1
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

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Jun 20, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same as above.

@BrucePay Bruce Payette (BrucePay) Jun 20, 2018

Copy link
Copy Markdown
Collaborator Author

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).

@adityapatwardhan

Aditya Patwardhan (adityapatwardhan) commented Jun 21, 2018

Copy link
Copy Markdown
Member

pushed a commit with [Feature] to kick off all tests.

@adityapatwardhan
Aditya Patwardhan (adityapatwardhan) merged commit bbb4f2e into PowerShell:master Jun 22, 2018
@TravisEz13

Travis Plunk (TravisEz13) commented Jun 25, 2018

Copy link
Copy Markdown
Member

Aditya Patwardhan (@adityapatwardhan) Please make sure you check the PR title before merging. The Fix for #4520 should not be in the PR title.

@adityapatwardhan

Aditya Patwardhan (adityapatwardhan) commented Jun 25, 2018

Copy link
Copy Markdown
Member

Travis Plunk (@TravisEz13) Yes agreed, missed it.

msoxzw pushed a commit to msoxzw/quiet-windows-10 that referenced this pull request Aug 22, 2021
Sion
Waiting latest PowerShell instead of using internal function, and not…
cc6677e 
… affect current functionality

PowerShell/PowerShell#6597
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
Bruce Payette @adityapatwardhan
Fix for PowerShell#4520 '-ArgumentList should accept @() or $null' (P…
cf6a3af 
…owerShell#6597)

* Fix for PowerShell#4520 '-ArgumentList should accept @() or $null'
Removed the [ValidateIsNotNulOrEmpty] attribute from the parameter.

* Fixed null and @() empty list tests to not both test null

* [Feature] Fixed test issue on non-Windows; added code to suppress displaying new windows on Windows.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov Ilya (iSazonov) iSazonov left review comments
@adityapatwardhan Aditya Patwardhan (adityapatwardhan) adityapatwardhan approved these changes
@daxian-dbw Dongbo Wang (daxian-dbw) Awaiting requested review from daxian-dbw
@anmenaga Andrew (anmenaga) Awaiting requested review from anmenaga
@dantraMSFT Dan Travison (dantraMSFT) Awaiting requested review from dantraMSFT
@Francisco-Gamino Francisco Gamino (Francisco-Gamino) Awaiting requested review from Francisco-Gamino
@HemantMahawar Hemant Mahawar (HemantMahawar) Awaiting requested review from HemantMahawar
@JamesWTruher James Truher (JamesWTruher) Awaiting requested review from JamesWTruher
@joeyaiello Joey Aiello (joeyaiello) Awaiting requested review from joeyaiello
@mirichmo Mike Richmond (mirichmo) Awaiting requested review from mirichmo
@SteveL-MSFT Steve Lee (SteveL-MSFT) Awaiting requested review from SteveL-MSFT
@TravisEz13 Travis Plunk (TravisEz13) Awaiting requested review from TravisEz13
@vors sergei (vors) Awaiting requested review from vors
+1 more reviewer
@PaulHigin Paul Higinbotham (PaulHigin) PaulHigin approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@JamesWTruher James Truher (JamesWTruher)

@adityapatwardhan Aditya Patwardhan (adityapatwardhan)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Start-Process -ArgumentList should accept an empty array / the null collection and interpret that as "no arguments to pass"

8 participants

@BrucePay @markekraus @SteveL-MSFT @adityapatwardhan @TravisEz13 @PaulHigin @iSazonov @JamesWTruher