Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Add ValidateNullOrEmpty to -Name parameter of Get-Service#2542

Merged
vors merged 7 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
HemantMahawar:GetServiceNullNameCopy head branch name to clipboard
Oct 27, 2016
Merged

Add ValidateNullOrEmpty to -Name parameter of Get-Service#2542
vors merged 7 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
HemantMahawar:GetServiceNullNameCopy head branch name to clipboard

Conversation

@HemantMahawar

Copy link
Copy Markdown
Contributor

Fix for #2540

@msftclas

Copy link
Copy Markdown

Hi @HemantMahawar, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!


It looks like you're a Microsoft contributor (Hemant Mahawar). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

$global:PSDefaultParameterValues = $originalDefaultParameterValues
}
Context 'Check null or empty value to the -Name parameter' {
It 'Should throw if null is passed to -Name parameter' {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You can use the -Testcases parameter to shorten this test a bit, e.g.

$testCases =
    @{ data = $null; value = 'null' },
    @{ data = [String]::Empty, value = 'empty string' }

It 'Should throw if <value> is passed to -Name parameter' -TestCases $testCases {
    param($data)
    try { Get-Service -Name $data }
    catch [System.Management.Automation.ParameterBindingException] {$_.Exception.ErrorId | Should Be 'ParameterArgumentValidationError'}
}

It 'Should throw if <value> is passed to -Name parameter via pipeline' -TestCases $testCases {
    param($data)
    try { $data | Get-Service }
    catch [System.Management.Automation.ParameterBindingException] {$_.Exception.ErrorId | Should Be 'ParameterArgumentValidationError'}
}

@HemantMahawar

Copy link
Copy Markdown
Contributor Author

@lzybkr Thanks for the great tip. Incorporated your feedback.

@vors

vors commented Oct 27, 2016

Copy link
Copy Markdown
Collaborator

@HemantMahawar it looks like GitHub cannot attribute commits to you. It's done via email on the commits. Can you add this email address to your email addresses in the GitHub settings?

@vors vors self-assigned this Oct 27, 2016
@HemantMahawar

Copy link
Copy Markdown
Contributor Author

@vors Done

It 'Should throw if <value> is passed through pipeline to -Name parameter' -TestCases $testCases {
param($data)
try {$data | Get-Service -ErrorAction Stop}
catch [System.Management.Automation.ParameterBindingException] {$_.Exception.ErrorId | Should Be 'ParameterArgumentValidationError'}

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.

Please, use Should Throw instead.
The current code will report success, if no error is thrown (the only check happens in the catch statement).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oops, I missed that - the pattern is to throw "Expected error on previous command" at the end of the try when you want to be specific about the exception thrown.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@vors @lzybkr Thanks for pointing out the holes in test (I should have caught them by running on PS 5.1 :)). Fixed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@vors @lzybkr Thanks for pointing out the holes in test (I should have caught them by running on PS 5.1 :)). Fixed


$testCases =
@{ data = $null; value = 'null' },
@{ data = [String]::Empty; value = 'empty string' }

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.

Where is value used in the test cases?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Isn't <value> in the description a reference to the variable?

Also followed the recommended practise to check for
FullyQualifiedErrorId
@HemantMahawar

Copy link
Copy Markdown
Contributor Author

@vors @lzybkr Thanks for pointing out the holes in test (I should have caught them by running on PS 5.1 :)). Fixed

@vors vors merged commit 6763854 into PowerShell:master Oct 27, 2016
@HemantMahawar HemantMahawar deleted the GetServiceNullName branch November 4, 2016 06:43
@SteveL-MSFT SteveL-MSFT added the Breaking-Change breaking change that may affect users label Feb 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Breaking-Change breaking change that may affect users

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.