Wrong expected exception assertion in tests by 2xmax · Pull Request #2908 · PowerShell/PowerShell

2xmax · Dec 20, 2016

Hello, this is a follow-up pull request to the comment I left about expected exception assertions in the current tests.

To put it briefly, there were two problems I found:

It was forgotten to throw an exception (and calibrate the test too;)) if no exception was thrown.

try
{
    Do-SomethingThatThrowsException
    # no throw "No Exception!" or something
}
catch
{
    $_.FullyQualifiedErrorId | should be "SomeErrorId"
}

As a result, in the case of no exception, the test would not become red. This command shows you lots mistakes like this:

grep -Hn -B 4 -ir "FullyQualifiedErrorId.*should" --include *Test*.ps1 --group-separator=$'\n\n---------------------------------\n\n'

powershell/engine/ParameterBinding/ParameterBinding.Tests.ps1-17- test-PositionalBinding1 1
powershell/engine/ParameterBinding/ParameterBinding.Tests.ps1-18- }
powershell/engine/ParameterBinding/ParameterBinding.Tests.ps1-19- catch
powershell/engine/ParameterBinding/ParameterBinding.Tests.ps1-20- {
powershell/engine/ParameterBinding/ParameterBinding.Tests.ps1:21: $_.FullyQualifiedErrorId | should be "AmbiguousPositionalParameterNoName,test-PositionalBinding1"

---------------------------------

powershell/engine/ParameterBinding/ParameterBinding.Tests.ps1-66- test-allownullattributes -Parameter2 1 -Parameter3 $null -ShowMe 1
powershell/engine/ParameterBinding/ParameterBinding.Tests.ps1-67- }
powershell/engine/ParameterBinding/ParameterBinding.Tests.ps1-68- catch
powershell/engine/ParameterBinding/ParameterBinding.Tests.ps1-69- {
powershell/engine/ParameterBinding/ParameterBinding.Tests.ps1:70: $_.FullyQualifiedErrorId | should be "ParameterArgumentValidationErrorEmptyStringNotAllowed,test-allownullattributes"

---------------------------------

powershell/engine/ParameterBinding/ParameterBinding.Tests.ps1-91- test-namedwithboolishargument -Parameter2 -Parameter1
powershell/engine/ParameterBinding/ParameterBinding.Tests.ps1-92- }
powershell/engine/ParameterBinding/ParameterBinding.Tests.ps1-93- catch
powershell/engine/ParameterBinding/ParameterBinding.Tests.ps1-94- {
powershell/engine/ParameterBinding/ParameterBinding.Tests.ps1:95: $_.FullyQualifiedErrorId | should be "MissingArgument,test-namedwithboolishargument"

---------------------------------

powershell/engine/ParameterBinding/ParameterBinding.Tests.ps1-150- test-singleintparameter -Parameter1 'dookie'
powershell/engine/ParameterBinding/ParameterBinding.Tests.ps1-151- }
powershell/engine/ParameterBinding/ParameterBinding.Tests.ps1-152- catch
powershell/engine/ParameterBinding/ParameterBinding.Tests.ps1-153- {
powershell/engine/ParameterBinding/ParameterBinding.Tests.ps1:154: $_.FullyQualifiedErrorId | should be "ParameterArgumentTransformationError,test-singleintparameter"

---------------------------------

powershell/engine/ParameterBinding/ParameterBinding.Tests.ps1-268- test-nameconflicts6 -Parameter2 1
powershell/engine/ParameterBinding/ParameterBinding.Tests.ps1-269- }
powershell/engine/ParameterBinding/ParameterBinding.Tests.ps1-270- catch
powershell/engine/ParameterBinding/ParameterBinding.Tests.ps1-271- {
powershell/engine/ParameterBinding/ParameterBinding.Tests.ps1:272: $_.FullyQualifiedErrorId | should be "ParameterNameConflictsWithAlias"

---------------------------------
powershell/Modules/Microsoft.PowerShell.Core/TestGetCommand.Tests.ps1-246- Get-Command testgetcommand-dynamicparametersdcr -testtorun returnduplicateparameter -ErrorAction SilentlyContinue
powershell/Modules/Microsoft.PowerShell.Core/TestGetCommand.Tests.ps1-247- }
powershell/Modules/Microsoft.PowerShell.Core/TestGetCommand.Tests.ps1-248- catch
powershell/Modules/Microsoft.PowerShell.Core/TestGetCommand.Tests.ps1-249- {
powershell/Modules/Microsoft.PowerShell.Core/TestGetCommand.Tests.ps1:250: $_.FullyQualifiedErrorId | Should Be "GetCommandMetadataError,Microsoft.PowerShell.Commands.GetCommandCommand"

---------------------------------

powershell/Modules/Microsoft.PowerShell.Security/ExecutionPolicy.Tests.ps1-920- try {
powershell/Modules/Microsoft.PowerShell.Security/ExecutionPolicy.Tests.ps1-921- Set-ExecutionPolicy -Scope $policyScope -ExecutionPolicy Restricted
powershell/Modules/Microsoft.PowerShell.Security/ExecutionPolicy.Tests.ps1-922- }
powershell/Modules/Microsoft.PowerShell.Security/ExecutionPolicy.Tests.ps1-923- catch {
powershell/Modules/Microsoft.PowerShell.Security/ExecutionPolicy.Tests.ps1:924: $_.FullyQualifiedErrorId | Should Be "CantSetGroupPolicy,Microsoft.PowerShell.Commands.SetExecutionPolicyCommand"

---------------------------------

powershell/Modules/Microsoft.PowerShell.Security/SecureString.Tests.ps1-15- try {
powershell/Modules/Microsoft.PowerShell.Security/SecureString.Tests.ps1-16- ConvertFrom-SecureString -secureString $null -key $null
powershell/Modules/Microsoft.PowerShell.Security/SecureString.Tests.ps1-17- }
powershell/Modules/Microsoft.PowerShell.Security/SecureString.Tests.ps1-18- catch {
powershell/Modules/Microsoft.PowerShell.Security/SecureString.Tests.ps1:19: $_.FullyQualifiedErrorId | should be "ParameterArgumentValidationErrorNullNotAllowed,Microsoft.PowerShell.Commands.ConvertFromSecureStringCommand"

---------------------------------

powershell/Modules/Microsoft.PowerShell.Utility/Export-Alias.Tests.ps1-104- try{
powershell/Modules/Microsoft.PowerShell.Utility/Export-Alias.Tests.ps1-105- Export-Alias $fulltestpath abcd02
powershell/Modules/Microsoft.PowerShell.Utility/Export-Alias.Tests.ps1-106- }
powershell/Modules/Microsoft.PowerShell.Utility/Export-Alias.Tests.ps1-107- catch{
powershell/Modules/Microsoft.PowerShell.Utility/Export-Alias.Tests.ps1:108: $_.FullyQualifiedErrorId | Should be "FileOpenFailure,Microsoft.PowerShell.Commands.ExportAliasCommand"

---------------------------------

powershell/Modules/Microsoft.PowerShell.Utility/Format-Wide.Tests.ps1-29- Format-Wide -InputObject $(Get-ChildItem) -Property CreationTime -View aoeu
powershell/Modules/Microsoft.PowerShell.Utility/Format-Wide.Tests.ps1-30- }
powershell/Modules/Microsoft.PowerShell.Utility/Format-Wide.Tests.ps1-31- catch
powershell/Modules/Microsoft.PowerShell.Utility/Format-Wide.Tests.ps1-32- {
powershell/Modules/Microsoft.PowerShell.Utility/Format-Wide.Tests.ps1:33: $_.FullyQualifiedErrorId | Should be "FormatCannotSpecifyViewAndProperty,Microsoft.PowerShell.Commands.FormatWideCommand"

Unclear error reporting if no exception was thrown (Expected: {SomeException}, But was: {})

powershell/Modules/Microsoft.PowerShell.Management/TimeZone.Tests.ps1-156-
powershell/Modules/Microsoft.PowerShell.Management/TimeZone.Tests.ps1-157- It "Call Set-TimeZone with invalid Id" {
powershell/Modules/Microsoft.PowerShell.Management/TimeZone.Tests.ps1-158- $exception = $null
powershell/Modules/Microsoft.PowerShell.Management/TimeZone.Tests.ps1-159- try { Set-TimeZone -Id "zzInvalidID" } catch { $exception = $_ }
powershell/Modules/Microsoft.PowerShell.Management/TimeZone.Tests.ps1:160: $exception.FullyQualifiedErrorID | Should Be "TimeZoneNotFound,Microsoft.PowerShell.Commands.SetTimeZoneCommand"

---------------------------------

powershell/Modules/Microsoft.PowerShell.Management/TimeZone.Tests.ps1-179-
powershell/Modules/Microsoft.PowerShell.Management/TimeZone.Tests.ps1-180- It "Call Set-TimeZone with invalid Name" {
powershell/Modules/Microsoft.PowerShell.Management/TimeZone.Tests.ps1-181- $exception = $null
powershell/Modules/Microsoft.PowerShell.Management/TimeZone.Tests.ps1-182- try { Set-TimeZone -Name "zzINVALID_Name" } catch { $exception = $_ }
powershell/Modules/Microsoft.PowerShell.Management/TimeZone.Tests.ps1:183: $exception.FullyQualifiedErrorID | Should Be "TimeZoneNotFound,Microsoft.PowerShell.Commands.SetTimeZoneCommand"

P.S. We can also discuss how to prevent oversights like this in the future except for careful code-review, like using static code analysis or common fixtures

…/github.com/PowerShell/PowerShell/blob/611f7f9b77f7bb336bc4c4069135eac8520bb77a/docs/testing-guidelines/WritingPesterTests.md#L54

msftclas · Dec 20, 2016

Hi @2xmax, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

iSazonov · Dec 20, 2016

@2xmax Seems Travis-CI temporary failed. Try restart CI job (ex. resubmit commit).

2xmax · Dec 20, 2016

closed to rerun travis

msftclas · Dec 20, 2016

Hi @2xmax, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

msftclas · Dec 20, 2016

Hi @2xmax, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

iSazonov · Dec 22, 2016

@2xmax Just added another erroneous test. Maybe you fix this too?

mirichmo · Jan 6, 2017

@2xmax Thanks for cleaning up the tests

fixed some expected exception tests according to the template https:/…

e29ddf7

…/github.com/PowerShell/PowerShell/blob/611f7f9b77f7bb336bc4c4069135eac8520bb77a/docs/testing-guidelines/WritingPesterTests.md#L54

msftclas added the cla-already-signed label Dec 20, 2016

2xmax changed the title ~~fixed some expected exception tests according to the template https:/…~~ Wrong expected exception handling in tests Dec 20, 2016

2xmax changed the title ~~Wrong expected exception handling in tests~~ Wrong expected exception assertion in tests Dec 20, 2016

2xmax closed this Dec 20, 2016

2xmax reopened this Dec 20, 2016

msftclas added the cla-already-signed label Dec 20, 2016

PowerShellTeam added the Review - Needed The PR is being reviewed label Dec 20, 2016

2xmax closed this Dec 20, 2016

2xmax reopened this Dec 20, 2016

msftclas added the cla-already-signed label Dec 20, 2016

mirichmo approved these changes Jan 6, 2017

View reviewed changes

mirichmo merged commit 1ccd155 into PowerShell:master Jan 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wrong expected exception assertion in tests#2908

Wrong expected exception assertion in tests#2908
mirichmo merged 1 commit into
PowerShell:masterPowerShell/PowerShell:masterfrom
2xmax:exp-ex-testsfixes2xmax/PowerShell:exp-ex-testsfixesCopy head branch name to clipboard

2xmax commented Dec 20, 2016 •

edited

Loading

Uh oh!

msftclas commented Dec 20, 2016

Uh oh!

iSazonov commented Dec 20, 2016

Uh oh!

2xmax commented Dec 20, 2016

Uh oh!

msftclas commented Dec 20, 2016

Uh oh!

msftclas commented Dec 20, 2016

Uh oh!

iSazonov commented Dec 22, 2016

Uh oh!

mirichmo commented Jan 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

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

Conversation

2xmax commented Dec 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msftclas commented Dec 20, 2016

Uh oh!

iSazonov commented Dec 20, 2016

Uh oh!

2xmax commented Dec 20, 2016

Uh oh!

msftclas commented Dec 20, 2016

Uh oh!

msftclas commented Dec 20, 2016

Uh oh!

iSazonov commented Dec 22, 2016

Uh oh!

mirichmo commented Jan 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

2xmax commented Dec 20, 2016 •

edited

Loading