Add Ability to Override Properties on New-MockObject

[aolszowka]

PR Summary

When @adbertram added New-MockObject back on #635 the long term plan was to add the ability to overwrite properties on the mocked object (See https://powershell.org/forums/topic/mocked-object-properties/) as well as methods.

This helps in scenarios where you are trying to mock out objects that have read-only properties. For example the ADUser object returned by Get-ADUser has several read-only properties that would be great to mock out (Name / Sid / etc).

This PR implements changes proposed by Monte Hazboun @Monte-Hazboun in the above linked thread. However the API I chose to use was slightly different (using Properties as opposed to Property / limited to only mocking out Properties as opposed to functions as @adbertram envisioned)

I have added simple unit tests using the System.Diagnostics.Process Object, one test showing how the existing mocking does not allow you to modify the Id property and another one utilizing the proposed changes.

I added inline documentation, however it is unclear to me if the documentation for the website is generated off of these examples, I would be happy to update this PR to include that.

Fix #706 (we did all we reasonably can)
Fix

PR Checklist

• PR has meaningful title
• Summary describes changes
• PR is ready to be merged
• Tests are added/update (if required)
• Documentation is updated/added (if required)

[aolszowka]

@nohwnd It looks like some of the Pipelines are stalled? Is there a way for me to re-trigger them?

[aolszowka]

The latest changes preserve the types that are being passed. Previously all of the types were converted to strings. I have also added a Unit Test that covers this scenario.

[nohwnd]

Need to have a proper look on this, adding to 5.2 but might merge in 5.3

[fflaten]

I'm not a developer by day so please correct me, but I didn't immediately find many other testing/mock frameworks supporting this use case (overwriting readonly properties).

The current implementation feels a bit dangerous to be an official API, but that might just be me. Could easily be added as a local helper function in your own repo where you have control over every scenario where it's used.

[fflaten]

Would personally place this as a context-block inside the existing Describe as it only groups specific test related to the new parameter in an existing function.

[aolszowka]

Can you give an example of what this might look like?

[fflaten]

Simply change the Describe with Context-keyword and move it inside the existing Describe-scriptblock + maybe update the block name.

Describe 'New-MockObject' {
    It 'instantiates an object from a class with no public constructors' {
        $type = 'Microsoft.PowerShell.Commands.Language'
        New-MockObject -Type $type | should -beoftype $type
    }

    Context 'Modifying readonly-properties' {
        # System.Diagnostics.Process.Id is normally read only, using the
        # Properties switch in New-MockObject should allow us to mock these.
        It 'Fails with just a normal mock' {
            ....
        }
        ....
    }
}

[aolszowka]

I believe I have resolved this with c866694

[aolszowka]

@fflaten

testing/mock frameworks supporting this use case (overwriting readonly properties)

You are correct at least with Moq (a popular C# mocking framework) at least as of Moq4, will not allow this (in fact they explicitly throw an exception). This is because most of what occurs with mocking is assumed to be working against an Interface.

However the industry as a whole has not completely adopted this design practice and there are decades of legacy code which will most likely never see this. In the particular example above I am using System.Diagnostics.Processing however in production we're trying to deal with AD Objects which are usually just light wrappers around old school COM Based management objects. There are hundreds, if not thousands of other examples of this (We are using this in production today and SqlManagement [which wraps SMO] is another offender).

In C# land we would get very creative relying on implementation level details to get what we want. My personal approach would be to first see if there is a non-public API that can be invoked to set the property in question.

For Process object in particular this is the case, the following C# Snippet shows how this can be leveraged:

Process p = new Process();

//Console.WriteLine(p.Id); // If you attempt to access this you will get an exception because no process has been associated yet.

// p.Id = 155; // This line throws a compiler exception because you cannot assign this.

// Rather we need to go behind the scenes and set it
MethodInfo processIdSetter = p.GetType().GetMethod("SetProcessId", BindingFlags.NonPublic | BindingFlags.Instance);
processIdSetter.Invoke(p, new object[] { 155 });

// This will print 155
Console.WriteLine(p.Id);

In this case this will do what we want.

In cases where we do not even have that level of access the next level of depth would be to mock out the private backing member itself, this is fraught with even more danger, because you are now getting deeper into understanding the implementation details of the object in question.

To use our process example above, not only do I need to set processId (which is the backing field for the public property Id) I also need to set another private field haveProcessId to fake out enough of the object to interact with it:

Process p = new Process();

//Console.WriteLine(p.Id); // If you attempt to access this you will get an exception because no process has been associated yet.

// p.Id = 255; // This line throws a compiler exception because you cannot assign this.

// Rather we need to go behind the scenes and set it
FieldInfo processIdProp = p.GetType().GetField("processId", BindingFlags.NonPublic | BindingFlags.Instance);
FieldInfo haveProcessIdProp = p.GetType().GetField("haveProcessId", BindingFlags.NonPublic | BindingFlags.Instance);

processIdProp.SetValue(p, 255);
haveProcessIdProp.SetValue(p, true);

// This will print 255
Console.WriteLine(p.Id);

In PowerShell land this is a little easier because PowerShell already has an implicit wrapper around any of the .NET Objects we are returning. As you have pointed out all we're doing is adding NoteProperties.

Where this becomes an issue is when you attempt to pass this "mocked object" to another C# type, because this mocking has not flowed back to the C# side (I cover this further down).

feels a bit dangerous to be an official API, but that might just be me

Completely agree, however you are in a testing framework in a mocking scenario where-in, as Raymond Chen would say, "Because programmers were trusted to do the right thing" (https://devblogs.microsoft.com/oldnewthing/20060216-06/?p=32263)

A "more correct way" would be to implement the C# mapping that I have done above, however this is extremely time consuming and would need to be customized for each object type used.

Rather the implementation as it stands today is a good example of an "80% Solution", embodying the principal of "Perfect is the enemy of good" (https://en.wikipedia.org/wiki/Perfect_is_the_enemy_of_good)

Often times in Industry you will need to do things that are not perfect to achieve design goals that are good enough or meets the needs of the problem at hand.

Could easily be added as a local helper function in your own repo where you have control over every scenario where it's used.

Here is where I will completely disagree.

This pattern, being fraught with danger, it better served with an official implementation that is well understood, along with its limitations. Because it is a part of the project, and is better understood by a wider range of developers, it is more likely that its usage will be correct in the 80% case, with the 20% case having a path forward.

This would be opposed to various one off versions, in which you are unlikely to get it right, or worse yet have it work in the 80% of scenarios without help from StackOverflow or the programming community at large when it goes south on you.

I am more than happy to formalize this into more extensive documentation that calls out the pitfalls for Developers who wish to reach for this within the Pester framework. I would say that this is a common enough scenario that I would have expected this out of the box, hence why we opened this PR.

[fflaten]

@aolszowka Thanks for the summary. As mentioned, I understand when and why you'd like this. The csharp-way, using SetValue() (when possible), is also too much of a treasure hunt per class to be something we can provide or even explain.

My concerns are mostly about the expectations of support with this. Add-Member is easy to use and explain for those who'd need it. Maybe the online docs could even provide an example including a warning. However as soon as it's part of the public functions, then the support/"bug" issues will come and they may be hard to diagnose and identify.

@nohwnd Thoughts?

[aolszowka]

@fflaten

Add-Member is easy to use and explain for those who'd need it.

I do not agree with this.

We can actually see it right here in this PR, the original implementation (as proposed on PowerShell.org) originally cast-ed all of the elements into strings, leaving a footgun that was eventually found by us once we transitioned into the SMO library. This was corrected and a unit test added, but is an easy trap to fall prey to.

Maintaining a consistent interface using the built-in will allow you to quickly identify the issue.

However as soon as it's part of the public functions, then the support/"bug" issues will come and they may be hard to diagnose and identify.

Completely agree, however until a consistent implementation is publicly facing and used by more than just a handful of developers we can't find the issues with everyone's artisanal implementations. I for one think it is better to follow the C Library mantra of a limited, but well understood library of functions with sometimes sub-optimal reference implementations. When you start running up against the wall its well trodden path with plenty of others who either:

Know how to manipulate the reference implementation to accomplish your task.
-or-
Can point you to where you need to go next.

We've already identified several potential issues between the two of us, however in our experience this has given us enough to solve actual business issues.

[fflaten]

I see your point. Waiting for the maintainer's vote here. 🙂
We should at least include a quick type-check to validate the most obvious user-error.

Not sure if it impacts performance much, but Add-Member supports a -NotePropertyMembers <IDictionary> -Force for PS3.0+ which could reduce some overhead of calling Add-Member multiple times. Would expect it to be safe after validating all the values for correct type.

[nohwnd]

I agree with @aolszowka, while C# test frameworks don't allow you to change the shape of objects e.g. change readonly to non-readonly, because this is limitation of the language. PowerShell is more dynamic in this regard, and I think it's perfectly okay to do this. I also agree with the other arguments, such as shipping one understood implementation of this that fits most of the cases. And even though I would rather see this shipped in a different module, I can see on my module Assert, that people are unwilling to install more than just Pester to do their testing.

I would like to expand this implementation to not only work for Properties on an empty object. It would be nice to have this for methods as well, and it would also be nice if we could capture the calls to the methods.

Here is an example I used in one of my presentations https://youtu.be/8GWqkGvV3H4?t=2518:

function Set-StandardizedComputerNameViaWmi ($Name) { 
    $n = $Name.ToUpperInvariant()
    $o = Get-WmiObject -Class Win32_ComputerSystem
    
    $returnCode = $o.Rename($n)
    
    1 -eq $returnCode
}

Describe 'cmdlet that returns object with behavior' {
    It "makes us fake the returned object, and store the method call values in it" {
        $rename = {
            param($Name) 
            
            # store the param value for further reference
            $this.__name = $Name

            # return proper return code to ensure
            # the function will pass
            1
        } 

        $p = [pscustomobject]@{
            __name = ''
        } | Add-Member -MemberType ScriptMethod -Name Rename -Value $rename -PassThru


        Mock Get-WmiObject {
            $p
        }
        
        Set-StandardizedComputerNameViaWmi -Name "mypc"

        # verify the behavior here, just like with Assert-MockCalled
        $p.__name | Should -BeExactly "MYPC"
    }
}

Here I "mock" a method on an object which captures the parameters to the call and returns a value. I think this could generalize nicely, and be useful in those rare cases where you need to do this.

But it's okay to implement just the Properties as long as we will at least prototype the Methods part and see that we are not doing something that would prevent us from implementing it later.

[nohwnd]

Not sure if it impacts performance much, but Add-Member supports a -NotePropertyMembers <IDictionary> -Force for PS3.0+ which could reduce some overhead of calling Add-Member multiple times. Would expect it to be safe after validating all the values for correct type.

The fastest way to add a member should be by editing the object properties directly.

Pester/src/Pester.RSpec.ps1

Line 127 in 890a21c

$e.PSObject.Properties.Add([Pester.Factory]::CreateNoteProperty("DisplayErrorMessage", [string]($r.Message -join [Environment]::NewLine)))

[nohwnd]

There is one more thing I wanted to explore and that is specifying getter and setter.

New-MockObject -Type 'System.Diagnostics.Process' -Properties @{Id = 123 }

New-MockObject -Type 'System.Diagnostics.Process' -Properties @{ Id = @{ get = { $this._id }; set = { $this._id = $value } }

Would having this ability be worth anything? Would there be any use cases? Am I overthinking it too much?

[nohwnd]

For the method mocking I envisioned something like this syntax:

# -- old syntax
$rename = {
    param($Name) 
    
    # store the param value for further reference
    $this.__name = $Name

    # return proper return code to ensure
    # the function will pass
    1
} 

$p = [pscustomobject]@{
    __name = ''
} | Add-Member -MemberType ScriptMethod -Name Rename -Value $rename -PassThru


Mock Get-WmiObject {
    $p
}

Set-StandardizedComputerNameViaWmi -Name "mypc"

# verify the behavior here, just like with Assert-MockCalled
$p.__name | Should -Be "mypc"


# --- new syntax
$p = New-MockObject -Methods @{ Rename = { param ($Name) 1 }  } -Type ...

Mock Get-WmiObject {
    $p
}

# act
Set-StandardizedComputerNameViaWmi -Name "mypc"

# assert
$p.CallHistory.Rename.Name | Should -Be "mypc"

We should consider how to provide mocks for multiple overloads. And some other scenarios when this would be useful.

I am also torn between @{ <membername> = <value> } syntax for both properties and methods (the one that is currently used) and a bit more verbose @{ Name = <membername>; Value = <value> }.

[nohwnd]

Anyone has any thoughts on this?

[aolszowka]

These look like good ideas to me, I'm out for another two weeks but I would be happy to work an implementation of the above when I'm back?

Getting method mocking would be extremely useful for us, especially with some of the more complex .Net mocks (ie SMO Database.Drop())

[nohwnd]

Sounds great :)

[fflaten]

Looks good. Would've liked it to support $this in methods, but not sure how to fix it without loosing call history.