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

Jump to bottom
Merged
adityapatwardhan merged 7 commits into from
Jun 22, 2018
Merged

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

adityapatwardhan merged 7 commits into from
Jun 22, 2018

Conversation

@BrucePay
Copy link
Collaborator

@BrucePay BrucePay commented Apr 9, 2018 •
edited by adityapatwardhan
Loading

Fix #4520

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

PR Summary

PR Checklist

iSazonov
iSazonov reviewed Apr 9, 2018
View reviewed changes
test/powershell/Modules/Microsoft.PowerShell.Management/Start-Process.Tests.ps1 Outdated
Copy link
Collaborator

@iSazonov iSazonov Apr 9, 2018 •
edited
Loading

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
Copy link
Contributor

markekraus commented Apr 12, 2018

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

@SteveL-MSFT
Copy link
Member

SteveL-MSFT commented Apr 12, 2018

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

For @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
iSazonov reviewed Apr 12, 2018
View reviewed changes
test/powershell/Modules/Microsoft.PowerShell.Management/Start-Process.Tests.ps1 Outdated
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.

@SteveL-MSFT
Copy link
Member

SteveL-MSFT commented Apr 25, 2018

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

@BrucePay
Copy link
Collaborator Author

BrucePay commented Apr 25, 2018

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

@BrucePay BrucePay force-pushed the brucepay_StartProcessArgs branch 3 times, most recently from d2da162 to 761748a Compare April 26, 2018 18:51
@BrucePay BrucePay force-pushed the brucepay_StartProcessArgs branch from 761748a to 22e1d11 Compare April 26, 2018 19:14
@BrucePay BrucePay force-pushed the brucepay_StartProcessArgs branch from 22e1d11 to 01603a5 Compare April 26, 2018 22:39
adityapatwardhan
adityapatwardhan requested changes Jun 20, 2018
View reviewed changes
Copy link
Member

@adityapatwardhan adityapatwardhan left a comment •
edited
Loading

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

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

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

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

@adityapatwardhan
Copy link
Member

adityapatwardhan commented Jun 21, 2018

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

@adityapatwardhan adityapatwardhan merged commit bbb4f2e into PowerShell:master Jun 22, 2018
@TravisEz13
Copy link
Member

TravisEz13 commented Jun 25, 2018

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

@adityapatwardhan
Copy link
Member

adityapatwardhan commented Jun 25, 2018

@TravisEz13 Yes agreed, missed it.

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

PowerShell/PowerShell#6597
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov iSazonov iSazonov left review comments

@adityapatwardhan adityapatwardhan adityapatwardhan approved these changes

@daxian-dbw daxian-dbw Awaiting requested review from daxian-dbw

@anmenaga anmenaga Awaiting requested review from anmenaga

@dantraMSFT dantraMSFT Awaiting requested review from dantraMSFT

@Francisco-Gamino Francisco-Gamino Awaiting requested review from Francisco-Gamino

@HemantMahawar HemantMahawar Awaiting requested review from HemantMahawar

@JamesWTruher JamesWTruher Awaiting requested review from JamesWTruher

@joeyaiello joeyaiello Awaiting requested review from joeyaiello

@mirichmo mirichmo Awaiting requested review from mirichmo

@SteveL-MSFT SteveL-MSFT Awaiting requested review from SteveL-MSFT

@TravisEz13 TravisEz13 Awaiting requested review from TravisEz13

@vors vors Awaiting requested review from vors

+1 more reviewer

@PaulHigin PaulHigin PaulHigin approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@JamesWTruher JamesWTruher

@adityapatwardhan 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