pester / Pester

Pester is the ubiquitous test and mock framework for PowerShell.
https://pester.dev/
Other
3.11k stars 473 forks source link

PesterThrow scriptblock won't redirect to null when testing PowerShell DSC exception handling #160

Closed anweiss closed 10 years ago

anweiss commented 10 years ago

The code in lines 11-13 of PesterThrow.ps1 (https://github.com/pester/Pester/blob/Beta/Functions/Assertions/PesterThrow.ps1#L11-L13) fails to redirect output to $null when testing PowerShell DSC exception handling. This subsequently causes the output to enter the pipeline and possibly break the test. Also, when a DSC exception is thrown by the user, multiple ErrorRecord objects are sent to the pipeline; a RuntimeException, which represents the user generated throw as well as a FailToProcessConfiguration exception that DSC throws. This prevents Pester from evaluating whether or not the exception's message is as asserted.

My proposal is to redirect all output to $null using *> $null.

dlwyatt commented 10 years ago

Please post the test code that you're using to trigger the problem; that'll help us make sure the problem is fixed, and stays that way in future releases.

anweiss commented 10 years ago

No prob. DockerClient is my DSC configuration.

Describe "DockerClient" {
    Context "when Hostname and Configuration parameters are null" { 
        It "should throw an exception" {
            Test-PositiveAssertion (PesterThrow { DockerClient })
        }

        It "should throw a specific exception message" {
            PesterThrow { DockerClient -ErrorVariable script:dockerClientArgError }
            $expectedErrorMessage = "Hostname and/or ConfigurationData must be specified"
            $result = Get-DoMessagesMatch $script:dockerClientArgError[1].Exception.Message $expectedErrorMessage
            $result | Should Be $True
        }
    }
}

I'm using the second It block as a workaround rather than trying to test for the specific error message in the Test-PositiveAssertion call.

I pulled the latest commits from beta and also getting a 'PesterThrow' is not recognized as the name of a cmdlet, function, ... error.

dlwyatt commented 10 years ago

Yeah, those are internal Pester functions, and one of the major changes in v3.0 was to isolate Pester's internals from the code under test. Here's what those tests should look like (though I'm not sure if you'll still encounter the problems you've described or not):

Describe "DockerClient" {
    Context "when Hostname and Configuration parameters are null" { 
        It "should throw an exception" {
            { DockerClient } | Should Throw
        }

        It "should throw a specific exception message" {
            { DockerClient } | Should Throw "Hostname and/or ConfigurationData must be specified"
        }
    }
}

Should is the public Pester command for performing Assertions. When you call Should Throw, it finds the PesterThrow function internally and takes care of that. It also determines whether to test for positive or negative assertions (you could have piped to Should Not Throw, for example.)

anweiss commented 10 years ago

ah, I had a feeling that's what was going on as I was digging through the source. So yea, my original issues still apply though...DSC spits out multiple ErrorRecords which causes the specific error message assertion to fail. In addition, the output is still sent to the pipeline.

dlwyatt commented 10 years ago

Where is DSC coming into the picture here? Is DockerClient a configuration name?

anweiss commented 10 years ago

Correct. https://github.com/anweiss/DockerClientDSC

dlwyatt commented 10 years ago

OK, cool. That should be everything I need to reproduce the problem and test your fix. I should be able to do that this evening.

anweiss commented 10 years ago

awesome, thanks!

dlwyatt commented 10 years ago

Well, this one kind of sucks, and I don't know if there's anything we can reasonably do about it from inside the Pester module (though there is a fairly convoluted way to make it work within your test code.) The main problem is that DSC catches terminating errors from configurations, and outputs them as non-terminating errors instead. On top of this, DSC configurations don't have the usual common parameters when you call them; you couldn't do, for example, DockerClient -ErrorAction Stop . The only way I've found to make this work is to set $ErrorActionPreference to Stop in the global scope (so DSC picks it up) before calling the configuration:

Describe 'Testing DSC Configuration error messages' {
    $errorMessage = 'Error message from configuration'
    configuration FakeConfig {
        throw $errorMessage
    }

    $scriptBlock = {
        $originalPreference = $global:ErrorActionPreference
        $global:ErrorActionPreference = 'Stop'

        try
        {
            FakeConfig
        }
        finally
        {
            $global:ErrorActionPreference = $originalPreference
        }
    }

    It "reports the correct error message from a configuration" {
        $scriptBlock | Should Throw $errorMessage
    }
}

Here are the relevant (and now slightly annoying) bits from DSC's configuration function:

Write-Debug "  ${name}: Evaluating configuration statement body..."
try
{
    # ... snip ...
    $result = $Body.InvokeWithContext($functionsToDefine, $variablesToDefine)
}
catch [System.Management.Automation.MethodInvocationException]
{
    Write-Debug "  ${name}: Top level exception: $($_ | out-string)"
    # Write the unwrapped exception message
    $PSCmdlet.CommandRuntime.WriteError((Get-InnerMostErrorRecord $_))
    Update-ConfigurationErrorCount
}

I don't think it's appropriate for us to fiddle with the global ErrorActionPreference for all calls to Should Throw, so it's probably up to the test scripts to handle that on an as-needed basis.

anweiss commented 10 years ago

Thanks @dlwyatt! Yea, agreed. For scenarios like this, it would be nice to have setUp() and tearDown() methods as in traditional TDD/BDD frameworks.

dlwyatt commented 10 years ago

Funny you should mention that. :) There's a pull request open to add those right now (though the keywords are currently named BeforeEach and AfterEach .) See https://github.com/pester/Pester/issues/157 and https://github.com/pester/Pester/pull/158

anweiss commented 10 years ago

Sweet! Yea that will definitely help organize scenarios such as this

anweiss commented 10 years ago

Also, DSC configs can still take the -ErrorAction parameter but it still affects the way Pester handles the error records

anweiss commented 10 years ago

@dlwyatt see #162 as well...another issue regarding DSC

dlwyatt commented 10 years ago

The latest changes have been merged into the Beta branch. Just ran this test again leveraging BeforeEach and AfterEach, and it looks good to me:

Describe 'Testing DSC Configuration error messages' {
    $errorMessage = 'Error message from configuration'
    configuration FakeConfig {
        throw $errorMessage
    }

    BeforeEach {
        $originalPreference = $global:ErrorActionPreference
        $global:ErrorActionPreference = 'Stop'
    }

    AfterEach {
        $global:ErrorActionPreference = $originalPreference
    }

    It "reports the correct error message from a configuration" {
        { FakeConfig } | Should Throw $errorMessage
    }
}
anweiss commented 10 years ago

Sweet! Thanks