pester / Pester

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

Mocking function with parameter validation still performs validation #734

Closed fromthewoods closed 5 years ago

fromthewoods commented 7 years ago

Having trouble understanding how mocking handles [ValidateScript] / [ValidatePattern]. It seems when I mock out a function that uses either of these Parameter options, Powershell still performs the validation and ruins the test.

function Invoke-Validation {
    [cmdletBinding()]
    Param (
        [Parameter(Mandatory=$true,
                   ValueFromPipelineByPropertyName=$false,
                   Position=0)]
        [ValidatePattern({^\D+$})]
        [string]$Word
    )

    Return $true
}

Describe -Name 'Invoke-Validation' {
    It 'should accept letters' {
        Invoke-Validation -Word 'things' | Should Be True
    }
    It 'should throw when not letters' {
        { Invoke-Validation -Word 123 } | Should Throw
    }
    It 'should not throw when mocked' {
        Mock -CommandName Invoke-Validation -MockWith { Return $true }
        Invoke-Validation -Word 123 | Should Be $true
    }
}

This returns:

Executing script D:\OneDrive\WindowsPowerShell\Tests\Test.Tests.ps1

  Describing Invoke-Validation
    [+] should accept letters 85ms
    [+] should throw when not letters 13ms
    [-] should not throw when mocked 20ms
      Cannot validate argument on parameter 'Word'. The argument "123" does not match the "^\D+$" pattern. Supply an argument that matches "^\D+$" and try the command again.
      at line: 23 in D:\OneDrive\WindowsPowerShell\Tests\Test.Tests.ps1
Tests completed in 119ms
Tests Passed: 2 Failed: 1 Skipped: 0 Pending: 0 Inconclusive: 0

The last test should be true but PowerShell seems to execute the validatepattern.

nohwnd commented 7 years ago

The mock copies the parameters of the function (except for common parameters), and so the validation on the mocked function are the same as on the original function. What is the real world case where you are running into this problem?

Typically you do want the mock to resemble the actual function as much as possible, because your goal is to make you code work with the actual function, not with a mock that has no restrictions.

fromthewoods commented 7 years ago

Thanks for your quick reply @nohwnd. I now understand how and why mocking a function and parameters happens.

I'm writing a module that wraps around Copy-Item to handle authentication for remote domains, among other things. Part of the function detection is to make sure the end user supplied a valid UNC path with an FQDN for the server name\share. A helper function strips out the FQDN and returns the domain of the server and prompts for credentials if the target domain is different than the local domain.

For unit testing, I want to do local copy actions to TestDrive:\ just to validate that a test file ends up on the simulated target, but that doesn't match the real world \\server.fqdn.local\share naming convention. I thought I'd be able to simply mock away my function and just return a (non UNC) string, but the ValidateScript/Pattern gets in the way.

nohwnd commented 7 years ago

Okay gotcha. At the moment there is no way to relax the rules on the mock parameters. The only way around it is to move the parameter validation to function code.

Jaykul commented 7 years ago

I don't think you should remove validation to make things testable -- there be dragons.

I'm a little confused about that real-world scenario though -- is the parameter validation that's blocking you on your Copy-Item wrapper, or on a helper function that you call? If it's on Copy-Item, then you obviously can't mock it out, but if it's on a validator, then you should just manually mock your function by defining a Mock-Verb-Noun function and using an alias:

Imagine your module was like:

New-Module PhonyModule {
    function Connect-Path
    {
        [CmdletBinding()]
        param([ValidatePattern("\\\\[^\\]*\\[^\\]*")]$Path)
        <# Get-Credential and other Stuff #>
        Write-Warning "Connected to $Path"
        $Path
    }

    function Copy-Item
    {
        [CmdletBinding()]
        param($Source, $Destination)
        if(Connect-Path $Destination)
        {
            Write-Progress "Copying $Source to $Destination"
            Microsoft.PowerShell.Management\Copy-Item $Source $Destination
            Sleep 1
        }
    }
} | Import-Module

If your test does something like:

It "Actually copies" { 
    Set-Content TestData:\OneFile "HELLO" 

    # Oh no, my test will fail because the destination isn't interesting enough
    Copy-Item TestDrive:\OneFile TestData:\AnotherFile
    Should -Exist TestDrive:\AnotherFile
}

Then you can fix it by alias mocking:


Describe "Alias Mocking" {
    # Why do these have to be global scope vs. module scope?
    function global:Mock-Connect-Path { Test-Path (Split-Path $args[0]) }
    Set-Alias Connect-Path Mock-Connect-Path -Scope global

    It "Actually copies" {        
        Set-Content TestDrive:\OneFile "HELLO"
        Copy-Item TestDrive:\OneFile TestDrive:\AnotherFile
        Should -Exist -ActualValue TestDrive:\AnotherFile
    }

    # Don't forget to clean up
    Remove-Item Alias:\Connect-Path
    Remove-Item Function:\Mock-Connect-Path
}
fromthewoods commented 7 years ago

Thanks @Jaykul! I will look into your suggestions.

jabbera commented 7 years ago

Any movement on this in 4.0? It's unpleasant.

Jaykul commented 7 years ago

What is unpleasant?

I don't actually know why this issue is still "open" -- I believe the actual answer to this is that is it performing as it was designed (on purpose and with great effort) to perform ...

jabbera commented 7 years ago

@Jaykul I've got a powershell module with a method on it (Call it Write-A). It has a [ValidationScript] on one of its parameters. The [ValidationScript] calls an internal function that does some stuff.

This module is being used from a script. While trying to mock Write-A in the script I don't want the validation called. I'm just trying to control the return value. I cannot figure out how to do without without moving my validation in to body of Write-A

fromthewoods commented 7 years ago

@jabbera Can you mock away the internal function that is called in the [ValidateScript]?

After thinking about this problem, as @Jaykul said, I don't think it's a good idea for Pester to remove input validation on mocked functions.

alx9r commented 7 years ago

@fromthewoods Nice suggestion. I just tested mocking away the function in [ValidateScript()]. It works. Here is the test:

function f
{
    param
    (
        [ValidateScript({$_|g})]
        $x
    )
    $x
}

function g
{
    param
    (
        [Parameter(ValueFromPipeline = $true)]
        $x
    )
    [bool]($x % 2)
}

Describe 'mock validation function' {
    It 'throws without mocking' {
        { f 2 } | Should throw 'validation script'
    }
    Context 'mock function and its validation function' {
        Mock f -Verifiable { 'return value' }
        Mock g -Verifiable { $true }
        It 'does not throw' {
            f 2 | Should be 'return value'
        }
        It 'records invokation' {
            Assert-MockCalled f 1 {
                $x -eq 2
            }
            Assert-MockCalled g 1 {
                $x -eq 2
            }
        }
    }
}

I currently don't have a strong opinion about whether Pester's current behavior is good or bad. But it's certainly possible to work around the problem by mocking the validation function.

jabbera commented 7 years ago

@fromthewoods @alx9r It doesn't work if the function is in a module! That's the problem. I'm not at my desk to make a repro but I'll try tonight.

alx9r commented 7 years ago

@jabbera Hmm...I just tested this with the functions in a module. Here is the test:

$guidFrag = [guid]::NewGuid().Guid.Split('-')[0]
$moduleName = "module-$guidFrag.psm1"
$modulePath = "$([System.IO.Path]::GetTempPath())\$moduleName"

{
    function f
    {
        param
        (
            [ValidateScript({$_|g})]
            $x
        )
        $x
    }

    function g
    {
        param
        (
            [Parameter(ValueFromPipeline = $true)]
            $x
        )
        [bool]($x % 2)
    }
} | Set-Content $modulePath

$module = Import-Module $modulePath -PassThru

InModuleScope $module.Name {
    Describe 'mock validation function' {
        It 'throws without mocking' {
            { f 2 } | Should throw 'validation script'
        }
        Context 'mock function and its validation function' {
            Mock f -Verifiable { 'return value' }
            Mock g -Verifiable { $true }
            It 'does not throw' {
                f 2 | Should be 'return value'
            }
            It 'records invokation' {
                Assert-MockCalled f 1 {
                    $x -eq 2
                }
                Assert-MockCalled g 1 {
                    $x -eq 2
                }
            }
        }
    }
}

That test passes. Perhaps there's an edge case that you're encountering. If that's the case I think it would be useful if you could post a repro of it here.

fromthewoods commented 7 years ago

@jabbera You'll need to use InModuleScope to access internal functions, as @alx9r notes above.

jabbera commented 7 years ago

@fromthewoods @alx9r that helped a little, but I can't figure out how nest InModuleScope. It seems as if it's only meant to be used for testing the module unless I'm missing something else.

See: https://github.com/jabbera/pester-issue/blob/master/Tests/Script1.test.ps1

For a trivial example. If I add another nested InModuleScope, I loose the scope of the first module.

Jaykul commented 7 years ago

I normally recommend against using InModuleScope when testing. Instead, use the -ModuleName parameter when creating mocks to make a specific mock affect things inside the module.

When you use InModuleScope you are invoking your code as though it was inside your module, which means that you can change variables, step on function names, and cause all sorts of mayhem intentionally or unintentionally :wink:

Additionally, using -ModuleName makes it clearer that there's no such thing as nesting InModuleScope: code cannot run in two scopes at once.

As a final note: you should normally not mock a function in two different modules in a single test -- it sort-of violates the principles of testing one thing at a time.

Say you're testing ModuleB, and you want to mock all calls to, say, Get-ChildItem -- but ModuleB calls ModuleA, which also calls Get-ChildItem... You could:

  1. Mock Get-ChildItem in both ModuleB and ModuleA -- by calling mock twice, using -ModuleName, or wrapping each mock in InModuleScope.
  2. Mock Get-ChildItem in ModuleB, and mock any calls to ModuleA, so that you're not testing ModuleA also...
jabbera commented 7 years ago

@Jaykul that was my first attempt, but I can't seem to override the ValidationScript methods with -ModuleName. PowerShell keeps saying the validation function can't be found. See newly commited code. (https://github.com/jabbera/pester-issue/commit/5a92b03bd5d8c619c7c3712f4ec3b2f5c9bb8b03)

Running the test results in the following:

PS C:\pester-issue\Tests> .\Script1.test.ps1

Describing this mock should work
  [-] this mock should work 534ms
    Cannot validate argument on parameter 'path'. The term 'TestFileA' is not recognized as the name of a cmdlet, functi
on, script file, or operable program. Check the spelling of the name, or if a path was included, verify that the path is
 correct and try again.
    at <ScriptBlock>, C:\pester-issue\Script.ps1: line 3
    3: Write-A "A"

Code inline for reference:

$env:PSModulePath = "..\Modules"
Import-Module "DemoModule" -Force
Import-Module "DemoModuleTwo" -Force
Import-Module "Pester" -Force

describe "this mock should work" {
    it "this mock should work" {
        Mock "Write-A" 
        Mock -ModuleName DemoModule "TestFileA" { $true }
        Mock "Write-B" 
        Mock -ModuleName DemoModuleTwo "TestFileB" { $true }            
        & "..\Script.ps1"
    }
}

As for only putting one thing under test at a time, that's exactly what I'm trying to do. I want my script to be the only thing under test. These modules are wrappers around external platforms that devs might not even have access to. We use Pester to mock the interaction.

Cheers, Mike

alx9r commented 7 years ago

@jabbera Could you put together a minimal but complete example that reproduces what you are seeing? The code you have posted does not include either the script under test or the modules it uses, so it's hard to make out where things are going wrong.

I think Script.ps1 is the "function" you have under test and it invokes functions in DemoModule and DemoModuleTwo either directly or indirectly. If those assumptions are correct, I'm pretty sure neither InModuleScope{} nor -ModuleName are necessary to mock calls from Script.ps1 because Script.ps1 is not bound to any module.

Based on the above reasoning, I changed the test that tries to reproduce your error. The test below creates two modules and a script file. The script file invokes a function in one of the modules. That function has a [ValidateScript()] attribute that invokes a function in the other module. Mocks for each of the functions in the modules are set up in the Context{} block, the script is invoked, and assertions are made that the mocks (not the real functions) were invoked. That all seems to work.

$guidFrag = [guid]::NewGuid().Guid.Split('-')[0]
$moduleName1 = "module1-$guidFrag.psm1"
$moduleName2 = "module2-$guidFrag.psm1"
$scriptName = "script-$guidFrag.ps1"
$modulePath1 = "$([System.IO.Path]::GetTempPath())\$moduleName1"
$modulePath2 = "$([System.IO.Path]::GetTempPath())\$moduleName2"
$scriptPath = "$([System.IO.Path]::GetTempPath())\$scriptName"

{
    function f
    {
        param
        (
            [ValidateScript({$_|g})]
            $x
        )
        $x
    }
} | Set-Content $modulePath1

{
    function g
    {
        param
        (
            [Parameter(ValueFromPipeline = $true)]
            $x
        )
        [bool]($x % 2)
    }
} | Set-Content $modulePath2

{
    f 2
} | Set-Content $scriptPath

Import-Module $modulePath1
Import-Module $modulePath2

Describe 'mock validation function' {
    It 'throws without mocking' {
        { f 2 } | Should throw 'validation script'
    }
    Context 'mock function and its validation function' {
        Mock f -Verifiable { 'return value' }
        Mock g -Verifiable { $true }
        It 'does not throw' {
            $r = & $scriptPath
            $r | Should be 'return value'
        }
        It 'records invokation' {
            Assert-MockCalled f 1 {
                $x -eq 2
            }
            Assert-MockCalled g 1 {
                $x -eq 2
            }
        }
    }
}
jabbera commented 7 years ago

@alx9r The repository: https://github.com/jabbera/pester-issue/ includes the entire copy of source code needed to reproduce the issue. All module source code is included. You should be able to clone and run: Test\Script1.test.ps1 to see the issue.

I should not need to mock TestFileA or TestFileB at all which is what I think you are saying, but if I don't I get an error that they cannot be found.

Commenting out: https://github.com/jabbera/pester-issue/blob/master/Tests/Script1.test.ps1#L9 and https://github.com/jabbera/pester-issue/blob/master/Tests/Script1.test.ps1#L11 like so:

$env:PSModulePath = "..\Modules"
Import-Module "DemoModule" -Force
Import-Module "DemoModuleTwo" -Force
Import-Module "Pester" -Force

describe "this mock should work" {
    it "this mock should work" {
        Mock "Write-A" 
        # Mock -ModuleName DemoModule "TestFileA" { $true }
        Mock "Write-B" 
        # Mock -ModuleName DemoModuleTwo "TestFileB" { $true }            
        & "..\Script.ps1"
    }
}

I get the following error:

Describing this mock should work
  [-] this mock should work 478ms
    Cannot validate argument on parameter 'path'. The term 'TestFileA' is not recognized as the name of a cmdlet, function, script file, or operable program. Check the spelling of the name, or if a path was included, verify that the path is correct and try again.
    at <ScriptBlock>, C:\pester-issue\Script.ps1: line 3
    3: Write-A "A"
jabbera commented 7 years ago

PS. Sorry for the delay. I was out for a week or so.

jabbera commented 7 years ago

What does work is the following which is totally ugly.

$env:PSModulePath = "..\Modules"
Import-Module "DemoModule" -Force
Import-Module "DemoModuleTwo" -Force
Import-Module "Pester" -Force

describe "this mock should work" {
    it "this mock should work" {
        function global:Mock-TestFile { $true }

        Set-Alias TestFileA Mock-TestFile -Scope global
        Set-Alias TestFileB Mock-TestFile -Scope global
        Mock "Write-A" 
        Mock "Write-B" 
        & "..\Script.ps1"
    }
}
alx9r commented 7 years ago

@jabbera I cloned your repo and took a look. I spotted a handful of problems and got it working by making these changes:

Change Script1.test.ps1 to the following

Import-Module "$PSScriptRoot\..\Modules\DemoModule" -Force
Import-Module "$PSScriptRoot\..\Modules\DemoModuleTwo" -Force
Import-Module "$PSScriptRoot\..\Modules\Pester" -Force

describe "this mock should work" {
    it "this mock should work" {
        Mock Write-A
        Mock TestFileA { $true }
        Mock Write-B
        Mock TestFileB { $true }
        & "$PSScriptRoot\..\Script.ps1"
    }
}

Export the Validation Functions

Change the FunctionsToExport lines in DemoModuleModule.psd1 and DemoModuleTwo.psd1 to

FunctionsToExport = 'Write-A','TestFileA'

and

FunctionsToExport = 'Write-B','TestFileB'

respectively.

Output

With those changes I get the following output when I invoke Script1.test.ps1:

Describing this mock should work
  [+] this mock should work 54ms
jabbera commented 7 years ago

@alx9r I see how that would work but now I'm making things that don't have to be public, public. It's a violation of the least access principal. I'd choose to embed the code in the function before I start to spam people's intellisense with functions they were never meant to call. I'm not trying to be difficult here, it's just no solution is great except pester just ignored these things or me being able to mock them easily:-)

Jaykul commented 7 years ago

Ok, I see: the problem is that the mock (and it's ValidateScript) is being created in Pester's scope, outside the module.

In other words: because the mock for Write-A is being redefined outside the DemoModule module, it can't call the private function that's used in the validate script.

You can solve that by just defining TestFileA (instead of mocking it):

    it "this mock should work" {
        Mock "Write-A"
        function TestFileA { $true }
        Mock "Write-B"
        function TestFileB { $true }
        & "$PSScriptRoot\..\Script.ps1"
    }

You could also solve it by mocking each function in their own module scope (where the validation function is defined):

    it "this mock should work" {
        InModuleScope DemoModule {
            Mock "Write-A"
            Mock "TestFileA" { $true }
        }
        InModuleScope DemoModuleTwo {
            Mock "Write-B"
            Mock "TestFileB" { $true }
        }
        & "$PSScriptRoot\..\Script.ps1"
    }

OR

    it "this mock should work" {
        Mock -ModuleName DemoModule "Write-A"
        Mock -ModuleName DemoModule "TestFileA" { $true }
        Mock -ModuleName DemoModuleTwo "Write-B"
        Mock -ModuleName DemoModuleTwo "TestFileB" { $true }
        & "$PSScriptRoot\..\Script.ps1"
    }
dlwyatt commented 7 years ago

Ok, I see: the problem is that the mock (and it's ValidateScript) is being created in Pester's scope, outside the module.

Hmm... we may be able to fix that. I'll tinker a bit.

it-praktyk commented 7 years ago

@dlwyatt , can I add it to the list of blockers?

nohwnd commented 7 years ago

@it-praktyk let's put this in 4.1, otherwise the new version won't ever be released.

it-praktyk commented 7 years ago

Thx for asking. Fine for me.

DarkLite1 commented 6 years ago

I'm stumbling upon the same issue. We have a module called Toolbox.HTML containing a function that sends out e-mails to end users. Something like this:

Function Send-MailHC {
    [CmdLetBinding()]
    Param (
        [parameter(Mandatory,Position=0)]
        [ValidateNotNullOrEmpty()]
        [String[]]$To,
        [parameter(Mandatory,Position=1)]
        [ValidateNotNullOrEmpty()]
        [String]$Subject,
        [parameter(Mandatory,Position=2,ValueFromPipeline)]
        [ValidateNotNullOrEmpty()]
        [String[]]$Message,
        [ValidateScript({Test-Path $_ -PathType Container})]
        [IO.DirectoryInfo]$LogFolder,
        [ValidateScript({Test-Path $_ -PathType Leaf})]
        [String[]]$Attachments
    )

    # Code
}

The problem when using Mock Send-MailHC is as described above, Pester is still executing the ValidateScript on the parameters Attachments and LogFolder. I'm not really willing to pull out the code in ValidateScript to a separate function just to be able to Mock it.

So my question: will the Mock functionality be expanded to ignore the ValidateScript parts of the parameters? It would greatly help us and simplify the code.

Thank you for an already great module. Really appreciate the hard work gone into this.

nohwnd commented 5 years ago

I think it is reasonable to allow the validation to be skipped, similarly to supressing the type in #601 it has similar advantages and disadvantages. I will mark this as Help wanted, the implementer should add a parameter to skip validation on a given parameter, ideally using -like wildcards.

renehernandez commented 5 years ago

@nohwnd I am thinking about working on this vs #601. Would you like me to prioritize one vs the other? Does it matter at all or does #601 could serve as basis for the resolution of this issue?

nohwnd commented 5 years ago

@renehernandez One or the other, in both cases you will be modifying the signature of the bootstrap function.

nohwnd commented 5 years ago

@renehernandez btw do you have twitter?

renehernandez commented 5 years ago

@nohwnd Yes I do, @renehr9102, but I haven't used in a like forever :)

nohwnd commented 5 years ago

Fixed in #1278