GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Test-Path: Return $false when given an empty or $null -Path/-LiteralPath value by vexx32 · Pull Request #8080 · PowerShell/PowerShell

Conversation

@vexx32

@vexx32 Rain Sallow (/u/ta11ow) (vexx32) commented Oct 19, 2018 •
edited by iSazonov
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

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

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • Change is not breaking
  • Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
  • This PR is ready to merge and is not Work in Progress.
    • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.
  • User-facing changes
    • Not Applicable
    • OR
    • User-facing Documentation needed
      • Issue filed - Issue link: Test-Path: Update documentation MicrosoftDocs/PowerShell-Docs#3165
  • Testing - New and feature
    • Not Applicable or can only be tested interactively
    • OR
    • Make sure you've added a new test if existing tests do not effectively test the code changed
      • Add [feature] if the change is significant or affects feature tests

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
KevinMarquette reacted with thumbs up emoji bergmeister reacted with hooray emoji
All reactions
  • 👍 1 reaction
  • 🎉 1 reaction
Rain Sallow (/u/ta11ow) (vexx32) added 2 commits October 18, 2018 22:48
@vexx32
Add empty / null path handling to Test-Path
1313095
@vexx32
Update tests to fail correctly under the sole remaining erroring cond…
7d5735a 
…ition
@vexx32
Rain Sallow (/u/ta11ow) (vexx32) requested review from Aditya Patwardhan (adityapatwardhan) and Dongbo Wang (daxian-dbw) as code owners October 19, 2018 03:21
iSazonov
Ilya (iSazonov) reviewed Oct 19, 2018
View reviewed changes
Comment thread src/Microsoft.PowerShell.Commands.Management/commands/management/PingPathCommand.cs Outdated
CmdletProviderContext currentContext = CmdletProviderContext;

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

@iSazonov Ilya (iSazonov) Oct 19, 2018

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

We usually use short code path:

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vexx32 Rain Sallow (/u/ta11ow) (vexx32) Oct 19, 2018

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
iSazonov reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@iSazonov

Ilya (iSazonov) commented Oct 19, 2018

Copy link
Copy Markdown
Collaborator

Steve Lee (@SteveL-MSFT) Is this a breaking change?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vexx32
Use quick-exit code paths instead of lengthy if/else
2f41783
@SteveL-MSFT Steve Lee (SteveL-MSFT) added Breaking-Change breaking change that may affect users Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Oct 19, 2018
@vexx32 Rain Sallow (/u/ta11ow) (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

Steve Lee (SteveL-MSFT) commented Oct 24, 2018

Copy link
Copy Markdown
Member

@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)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@SteveL-MSFT Steve Lee (SteveL-MSFT) added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Oct 24, 2018
@iSazonov

Ilya (iSazonov) commented Oct 25, 2018

Copy link
Copy Markdown
Collaborator

Rain Sallow (/u/ta11ow) (@vexx32) Please continue and open new issue in PowerShell-Docs.
vexx32 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov Ilya (iSazonov) self-assigned this Oct 25, 2018
@vexx32 Rain Sallow (/u/ta11ow) (vexx32) mentioned this pull request Oct 25, 2018
Test-Path: Update documentation MicrosoftDocs/PowerShell-Docs#3165
Closed
1 of 8 tasks
@vexx32
Add non-terminating errors for null input
44417f5 
$false will be output if it encounters a non-null but empty string.
@vexx32

Rain Sallow (/u/ta11ow) (vexx32) commented Oct 25, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator Author

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov

Ilya (iSazonov) commented Oct 25, 2018

Copy link
Copy Markdown
Collaborator

Rain Sallow (/u/ta11ow) (@vexx32) Please add tests for new behaviors.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Rain Sallow (/u/ta11ow) (vexx32) added 4 commits October 25, 2018 10:48
@vexx32
Reformat test file for readability
63e1b8f
@vexx32
Use more descriptive error ID
d793658
@vexx32
Add Tests for new behaviour
794e78b
@vexx32
Remove unnecessary blank line per StyleCop
cae5294
iSazonov
Ilya (iSazonov) reviewed Oct 25, 2018
View reviewed changes
Comment thread src/Microsoft.PowerShell.Commands.Management/commands/management/PingPathCommand.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread 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

@iSazonov Ilya (iSazonov) Oct 25, 2018

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vexx32 Rain Sallow (/u/ta11ow) (vexx32) Oct 25, 2018

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Oct 25, 2018

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Aditya Patwardhan (@adityapatwardhan) James Truher (@JamesWTruher) Steve Lee (@SteveL-MSFT) Can we use the pattern to test non-terminating errors?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Oct 25, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vexx32 Rain Sallow (/u/ta11ow) (vexx32) Oct 25, 2018

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Oct 26, 2018

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I believe we need to add a comment to describe the pattern.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vexx32 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread 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'

@iSazonov Ilya (iSazonov) Oct 25, 2018

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vexx32 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@vexx32 Rain Sallow (/u/ta11ow) (vexx32) Oct 30, 2018

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Rain Sallow (/u/ta11ow) (vexx32) added 5 commits October 30, 2018 16:30
@vexx32
Add new tests
60810ca 
Add comment to confusing but necessary test pattern
@vexx32
Clarify it name
e731fab
@vexx32
Fix endless loop caused by change of error type in tests
2008e51
@vexx32
Fix FullyQualifiedErrorID
28cc91f
@vexx32
Consolidate ErrorIDs to match
0be7bf1
@iSazonov

Ilya (iSazonov) commented Oct 31, 2018

Copy link
Copy Markdown
Collaborator

@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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vexx32

Rain Sallow (/u/ta11ow) (vexx32) commented Oct 31, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator Author

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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
iSazonov
Ilya (iSazonov) approved these changes Oct 31, 2018
View reviewed changes
@iSazonov

Ilya (iSazonov) commented Nov 6, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

Rain Sallow (/u/ta11ow) (@vexx32) Could you please continue?

Update: Sorry, seems my comments was addressed.
vexx32 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov

Ilya (iSazonov) commented Nov 14, 2018

Copy link
Copy Markdown
Collaborator

Steve Lee (@SteveL-MSFT) Aditya Patwardhan (@adityapatwardhan) Could you please review the small PR?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
SteveL-MSFT
Steve Lee (SteveL-MSFT) requested changes Nov 14, 2018
View reviewed changes
Comment thread src/Microsoft.PowerShell.Commands.Management/commands/management/PingPathCommand.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/Microsoft.PowerShell.Commands.Management/commands/management/PingPathCommand.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
Rain Sallow (/u/ta11ow) (vexx32) added 2 commits November 15, 2018 21:34
@vexx32
Add .resx file for TestPath
890fcb1
@vexx32
Use resource file for error string
cddfb79
@iSazonov Ilya (iSazonov) added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Nov 16, 2018
@vexx32

Rain Sallow (/u/ta11ow) (vexx32) commented Nov 16, 2018

Copy link
Copy Markdown
Collaborator Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov

Ilya (iSazonov) commented Nov 16, 2018

Copy link
Copy Markdown
Collaborator

Restart MacOs CI.
vexx32 reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
SteveL-MSFT
Steve Lee (SteveL-MSFT) approved these changes Nov 20, 2018
View reviewed changes
@iSazonov
Ilya (iSazonov) merged commit 775552c into PowerShell:master Nov 21, 2018
Ilya (iSazonov) pushed 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…
6317858 
…ath value (PowerShell#8080)
@vexx32
Rain Sallow (/u/ta11ow) (vexx32) deleted the Test-Path/AllowNull branch January 22, 2019 13:19
@ZenoArrow

ZenoArrow commented Nov 5, 2019

Copy link
Copy Markdown

Is this change due to be released in PowerShell 7? I haven't seen it in the release notes of the previews released to date, just wanted to check that it's still on course to be included.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov

Ilya (iSazonov) commented Nov 5, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

ZenoArrow The fix is in current 7.0 Preview5 and will be in 7.0 GA too.
Also the fix was mentioned in CHANGELOG.md.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ZenoArrow

ZenoArrow commented Nov 5, 2019

Copy link
Copy Markdown

Ilya (@iSazonov)
Thanks for the heads up about the changelog. Referencing that file...
"Test-Path: Return $false when given an empty or $null -Path/-LiteralPath value (#8080) (Thanks Rain Sallow (/u/ta11ow) (@vexx32)!)"

The behaviour seems to be fixed for empty path, but the behaviour is different from what I'd expect for $null values...

TestPath20191105

Am I missing something?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vexx32

Rain Sallow (/u/ta11ow) (vexx32) commented Nov 5, 2019

Copy link
Copy Markdown
Collaborator Author

ZenoArrow apologies! Looks like I neglected to update the PR description properly. The PS committee's decision was that we should maintain the error for $null values. I didn't agree, but that is that they decided and so that is how it ended up being implemented.

If you disagree with their decision there (I'm right there with you really) I'd recommend opening an issue so that they can see that the behaviour isn't as desired and we can have them reconvene and reassess. 🙂

/Cc Steve Lee (@SteveL-MSFT)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ZenoArrow

ZenoArrow commented Nov 6, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

Rain Sallow (/u/ta11ow) (@vexx32) Thanks for the heads up. I can raise a new issue, but it'd effectively just be a duplicate of the issue raised by Kevin Marquette (@KevinMarquette) last year...
#8076

Regarding returning the non-terminating error, I understand this to a degree, but what I can't understand is why Test-Path can't return more than just the error. Correct me if I'm wrong, but usually error stream output doesn't interfere with the standard output, so in my mind there's no reason not to return both (an error message to the error stream and a "False" value to the standard stream).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vexx32

Rain Sallow (/u/ta11ow) (vexx32) commented Nov 6, 2019

Copy link
Copy Markdown
Collaborator Author

It certainly could! But it's not strictly necessary. If you're using Test-Path as a conditional in most cases the non-respose will be treated as $false.

For example:

if (Test-Path $null) { "true!" } Else { "false!" }
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ZenoArrow

ZenoArrow commented Nov 6, 2019

Copy link
Copy Markdown

Rain Sallow (/u/ta11ow) (@vexx32) Good to know. In that case the current improvements to Test-Path should cover what I was hoping for.
vexx32 reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kilasuit Ryan Yates (kilasuit) mentioned this pull request Oct 22, 2024
test-path $null throws error #24468
Closed
5 tasks done
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@vexx32 @iSazonov
Test-Path: Return $false when given an empty or $null -Path/-LiteralP…
c58a224 
…ath value (PowerShell#8080)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SteveL-MSFT Steve Lee (SteveL-MSFT) SteveL-MSFT approved these changes
@iSazonov Ilya (iSazonov) iSazonov approved these changes
@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Awaiting requested review from adityapatwardhan
@daxian-dbw Dongbo Wang (daxian-dbw) Awaiting requested review from daxian-dbw

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@iSazonov Ilya (iSazonov)

Labels

Breaking-Change breaking change that may affect users CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log Committee-Reviewed PS-Committee has reviewed this and made a decision

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Test-Path -Path $null should return $false Test-Path returns $true for ' ' on Windows

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@vexx32 @iSazonov @SteveL-MSFT @ZenoArrow
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.