★ wanayoo — archive 1999 https://github.com/PowerShell/PowerShell/pull/8080Nouvelle recherche | Portail wanayoo
Skip to content

/PowerShell

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.

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

Test-Path: Return $false when given an empty or $null -Path/-LiteralPath value #8080

Merged
merged 15 commits into from Nov 21, 2018
+228 −49

Conversation

Reviewers

@SteveL-MSFT SteveL-MSFT

@iSazonov iSazonov

@daxian-dbw daxian-dbw

@adityapatwardhan adityapatwardhan

Assignees

@iSazonov iSazonov

Labels
Breaking-Change CL-General Committee-Reviewed
Projects
None yet
Milestone
No milestone
3 participants
@vexx32 @iSazonov @SteveL-MSFT
@vexx32
Copy link
Contributor

commented Oct 19, 2018 •
edited by iSazonov

PR Summary

Fix #5717.
Fix #8076.

Test-Path is expected to be almost exclusively a True/False response cmdlet, and the cases where it may error or return an unexpected result are a few too many at present (see above issues). This PR allows $null / empty value(s) to be passed to Test-Path without throwing parameter binding exceptions. It also adds additional logic to Test-Path such that passing it $null, '', or ' ' (or any pure whitespace string) returns $false instead of throwing an error or returning $true on a "path" that is nothing but whitespace.

New behaviour:

PS> Test-Path ' '
False
PS> Test-Path ''
False
PS> Test-Path @()
False

Error conditions:

PS> Test-Path $null
PS> Test-Path $null, $null

These are non-terminating errors.

A small handful of tests predicated on Test-Path failing in these cases have been updated to simply transform the error into terminating via -ErrorAction, as it appears that the important part of the test was not Test-Path itself, but throwing a terminating error in specific code strictures.

Tests for the additional behaviours have also been added.

PR Checklist

@vexx32 vexx32 requested review from adityapatwardhan and daxian-dbw as code owners Oct 19, 2018

@iSazonov

iSazonov reviewed Oct 19, 2018
View changes

src/Microsoft.PowerShell.Commands.Management/commands/management/PingPathCommand.cs Outdated
@@ -156,49 +162,60 @@ protected override void ProcessRecord()
{
CmdletProviderContext currentContext = CmdletProviderContext;

foreach (string path in _paths)
if (_paths != null && _paths.Length != 0)

This comment has been minimized.

Sign in to view
Copy link
@iSazonov

iSazonov Oct 19, 2018

Collaborator

We usually use short code path:

Suggested change
if (_paths != null && _paths.Length != 0)
if (_paths == null || _paths.Length == 0)
{
return false;
}

This comment has been minimized.

Sign in to view
Copy link
@vexx32

vexx32 Oct 19, 2018

Author Contributor

This is in the ProcessRecord() method, so returning false is not an option. I suppose WriteObject(false); return; is the most appropriate short-cut method here. Thank you!
@iSazonov

This comment has been minimized.

Sign in to view
Copy link
Collaborator

commented Oct 19, 2018

@SteveL-MSFT Is this a breaking change?

@SteveL-MSFT SteveL-MSFT added Breaking-Change Review - Committee labels Oct 19, 2018

@vexx32 vexx32 changed the title Add empty / null path handling to Test-Path Test-Path: Return $false when given an empty or $null -Path/-LiteralPath value Oct 19, 2018

@SteveL-MSFT

This comment has been minimized.

Sign in to view
Copy link
Member

commented Oct 24, 2018

@PowerShell/powershell-committee reviewed this, we believe the correct behavior is to accept null/nullcollection, but return non-terminating error at runtime (instead of exception or $false)

@SteveL-MSFT SteveL-MSFT added Committee-Reviewed and removed Review - Committee labels Oct 24, 2018

@iSazonov

This comment has been minimized.

Sign in to view
Copy link
Collaborator

commented Oct 25, 2018

@vexx32 Please continue and open new issue in PowerShell-Docs.

@iSazonov iSazonov self-assigned this Oct 25, 2018

@vexx32 vexx32 referenced this pull request Oct 25, 2018

Closed

Test-Path: Update documentation #3165

1 of 8 tasks complete
@vexx32
Add non-terminating errors for null input 
$false will be output if it encounters a non-null but empty string.
Loading status checks…
44417f5
@vexx32

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Oct 25, 2018 •
edited

Added non-terminating errors for the following cases, per PowerShell committee recommendation:

  1. Null collection handed to Test-Path (e.g., Test-Path $null)
  2. Collection of null items handed to Test-Path (e.g., Test-Path $null, $null)

$false will be output if:

  1. Path is invalid (same as existing behaviour)
  2. Path is empty or pure whitespace.

Docs issue: MicrosoftDocs/PowerShell-Docs#3165
@iSazonov

This comment has been minimized.

Sign in to view
Copy link
Collaborator

commented Oct 25, 2018

@vexx32 Please add tests for new behaviors.

vexx32 added some commits Oct 25, 2018

@vexx32
Reformat test file for readability
Loading status checks…
63e1b8f
@vexx32
Use more descriptive error ID
Loading status checks…
d793658
@vexx32
Add Tests for new behaviour
Loading status checks…
794e78b
@vexx32
Remove unnecessary blank line per StyleCop
Loading status checks…
cae5294
@iSazonov

iSazonov reviewed Oct 25, 2018
View changes

Show resolved Hide resolved ...ft.PowerShell.Commands.Management/commands/management/PingPathCommand.cs
test/powershell/Modules/Microsoft.PowerShell.Management/Test-Path.Tests.ps1

It 'Should write a non-terminating error when given a null path' {
{ Test-Path -Path $null -ErrorAction Stop } | Should -Throw -ErrorId 'NullPathNotPermitted'
{ Test-Path -Path $null -ErrorAction SilentlyContinue } | Should -Not -Throw

This comment has been minimized.

Sign in to view
Copy link
@iSazonov

iSazonov Oct 25, 2018

Collaborator

Seems we should remove this. I am not sure that it tests "non-terminating"

This comment has been minimized.

Sign in to view
Copy link
@vexx32

vexx32 Oct 25, 2018

Author Contributor

That's what I thought, too. Surprisingly, though, -ErrorAction SilentlyContinue will still trigger a | should -throw if and only if the error is terminating to start with (either throw or ThrowTerminatingError())

@indented-automation had to explain that one to me. :)

This comment has been minimized.

Sign in to view
Copy link
@iSazonov

iSazonov Oct 25, 2018

Collaborator

@adityapatwardhan @JamesWTruher @SteveL-MSFT Can we use the pattern to test non-terminating errors?

This comment has been minimized.

Sign in to view
Copy link
@SteveL-MSFT

SteveL-MSFT Oct 25, 2018

Member

Since this change is explicitly to be a non-terminating error, this pattern seems fine. But should probably be followed up with -ErrorAction Stop to verify the non-terminating error is correct.

This comment has been minimized.

Sign in to view
Copy link
@vexx32

vexx32 Oct 25, 2018

Author Contributor

I'm not sure I follow? I don't see a need to double-check it with -ErrorAction Stop

This comment has been minimized.

Sign in to view
Copy link
@iSazonov

iSazonov Oct 26, 2018

Collaborator

I believe we need to add a comment to describe the pattern.
test/powershell/Modules/Microsoft.PowerShell.Management/Test-Path.Tests.ps1 Outdated
}

It 'Should write a non-terminating error when given a null path' {
{ Test-Path -Path $null -ErrorAction Stop } | Should -Throw -ErrorId 'NullPathNotPermitted'

This comment has been minimized.

Sign in to view
Copy link
@iSazonov

iSazonov Oct 25, 2018

Collaborator

I believe we should tests here Test-Path $null, $null;.

This comment has been minimized.

Sign in to view
Copy link
@vexx32

vexx32 Oct 30, 2018

Author Contributor

Done. Also had to modify some ErrorPosition tests as they were causing infinite loops when the cmldet simply wrote a non-terminating error.

vexx32 added some commits Oct 30, 2018

@vexx32
Add new tests 
Add comment to confusing but necessary test pattern
Loading status checks…
60810ca
@vexx32
Clarify it name
Loading status checks…
e731fab
@vexx32
Fix endless loop caused by change of error type in tests
Loading status checks…
2008e51
@vexx32
Fix FullyQualifiedErrorID
Loading status checks…
28cc91f
@vexx32
Consolidate ErrorIDs to match
Loading status checks…
0be7bf1
@iSazonov

This comment has been minimized.

Sign in to view
Copy link
Collaborator

commented Oct 31, 2018

@PowerShell/powershell-committee reviewed this, we believe the correct behavior is to accept null/nullcollection, but return non-terminating error at runtime (instead of exception or $false)

We'll get $false without -ErrorAction Stop - yes?
@vexx32

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Oct 31, 2018 •
edited

We'll get a non-terminating error per the recommendation. In terms of output this will effectively be treated as $null in expressions and statements; in most reasonable circumstances this will be coerced to$falsein a statement that is expecting a boolean conditional, yes.

Would explicitly outputting $false along with the error(s) be preferable?
@iSazonov

iSazonov approved these changes Oct 31, 2018
View changes

@iSazonov

This comment has been minimized.

Sign in to view
Copy link
Collaborator

commented Nov 6, 2018 •
edited

@vexx32 Could you please continue?

Update: Sorry, seems my comments was addressed.
@iSazonov

This comment has been minimized.

Sign in to view
Copy link
Collaborator

commented Nov 14, 2018

@SteveL-MSFT @adityapatwardhan Could you please review the small PR?
@SteveL-MSFT

SteveL-MSFT requested changes Nov 14, 2018
View changes

Show resolved Hide resolved ...ft.PowerShell.Commands.Management/commands/management/PingPathCommand.cs Outdated
Show resolved Hide resolved ...ft.PowerShell.Commands.Management/commands/management/PingPathCommand.cs Outdated

vexx32 added some commits Nov 16, 2018

@vexx32
Add .resx file for TestPath
Loading status checks…
890fcb1
@vexx32
Use resource file for error string
Loading status checks…
cddfb79

@iSazonov iSazonov added the CL-General label Nov 16, 2018

@vexx32

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Nov 16, 2018

MacOS CI failure appears to be temporary and no related to the PR as far as I can see.
@iSazonov

This comment has been minimized.

Sign in to view
Copy link
Collaborator

commented Nov 16, 2018

Restart MacOs CI.
@SteveL-MSFT

SteveL-MSFT approved these changes Nov 20, 2018
View changes

@iSazonov iSazonov merged commit 775552c into PowerShell:master Nov 21, 2018
7 checks passed

7 checks passed

CodeFactor 2 issues fixed. 1 issue found.
Details
PowerShell-CI-linux #PR-8080-20181116.01 succeeded
Details
PowerShell-CI-macos #PR-8080-20181116.02 succeeded
Details
PowerShell-CI-spelling #PR-8080-20181116.01 succeeded
Details
PowerShell-CI-windows #PR-8080-20181116.01 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla All CLA requirements met.
Details

iSazonov added a commit to iSazonov/PowerShell that referenced this pull request Nov 29, 2018

@vexx32 @iSazonov
Test-Path: Return $false when given an empty or $null -Path/-LiteralP… 
…ath value (PowerShell#8080)
6317858

@vexx32 vexx32 deleted the vexx32:Test-Path/AllowNull branch Jan 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.