Stupid mock assertions in tests

[cezarypiatek]

There is a lot of stupid assertions in tests which test implementation details instead of behavior. It doesn't add any value but only makes refactoring harder and bloats the tests code (for example MSFT_xWebsiteTests.ps1 has almost 4k lines of code, it's really hard to manage that)

Example:

 It 'Should call all the mocks' {
                    Assert-MockCalled -CommandName Add-WebConfiguration -Exactly 1
                    Assert-MockCalled -CommandName Confirm-UniqueBinding -Exactly 1
                    Assert-MockCalled -CommandName Confirm-UniqueServiceAutoStartProviders -Exactly 1
                    Assert-MockCalled -CommandName Test-AuthenticationEnabled -Exactly 4
                    Assert-MockCalled -CommandName Test-WebsiteBinding -Exactly 1
                    Assert-MockCalled -CommandName Update-WebsiteBinding -Exactly 1
                    Assert-MockCalled -CommandName Update-DefaultPage -Exactly 1
                    Assert-MockCalled -CommandName Set-Authentication -Exactly 4
                    Assert-MockCalled -CommandName Get-Item -Exactly 3
                    Assert-MockCalled -CommandName Set-Item -Exactly 3
                    Assert-MockCalled -CommandName Set-ItemProperty -Exactly 9
                    Assert-MockCalled -CommandName Start-Website -Exactly 1
                    Assert-MockCalled -CommandName Set-WebConfigurationProperty -Exactly 2
                    Assert-MockCalled -CommandName Test-LogCustomField -Exactly 1
                }

I would like to propose to delete all that assertions add mention it as a bad practice in the Contribution guideline.

[PlagueHO]

Hi @cezarypiatek - Assert-MockCalled can definitely get a bit out-of-hand and agree that it takes us into testing implementation.

However, in some functions it is required as there is no other way to validate the behavior (especially if it is changing the system). That said, just checking a mock is called without validating the parameters it was called with isn't ideal either. So the above mock assertions definitely look like they could be improved.

Some guidance could definitely be used here. E.g. I would only be making assertions over the calls that actually change the system state - not worrying about the commands that are returning info. E.g. the example above really should be something like:

 It 'Should call all the mocks' {
                    Assert-MockCalled -CommandName Add-WebConfiguration -Exactly -Times 1 -ParameterFilter { ... TBC ...}
                    Assert-MockCalled -CommandName Update-WebsiteBinding -Exactly -Times 1 -ParameterFilter { ... TBC ...}
                    Assert-MockCalled -CommandName Update-DefaultPage -Exactly -Times 1 -ParameterFilter { ... TBC ...}
                    Assert-MockCalled -CommandName Set-Authentication -Exactly -Times 4 -ParameterFilter { ... TBC ...}
                    Assert-MockCalled -CommandName Set-Item -Exactly -Times 3 -ParameterFilter { ... TBC ...}
                    Assert-MockCalled -CommandName Set-ItemProperty -Exactly -Times 9 -ParameterFilter { ... TBC ...}
                    Assert-MockCalled -CommandName Start-Website -Exactly -Times 1 -ParameterFilter { ... TBC ...}
                    Assert-MockCalled -CommandName Set-WebConfigurationProperty -Exactly -Times 2 -ParameterFilter { ... TBC ...}
                }

That said, based on these assertions alone, I'd argue that this function is probably in serious need of refactoring - as it is "doing too much". But I guess that is what you're saying: refactoring is expensive due to this problem.

tl;dr; Agreed - need some guidelines in place. @johlju - your thoughts?

[johlju]

I think Assert-MockCalled is much needed to make sure the correct calls are made in the correct code path, to make sure what will happen actually happens. And it is also very important that the mock only be called once, so it does not enter into another code path and executes the the same call twice, if that is not expected.
But is should not be necessary that all calls need to have a ParameterFilter, I think that it depends on the different code paths that might exist. Only one code path, no need to have ParameterFilter, to code paths that makes the same call but with different parameters, then for sure we should have ParameterFilter to distinguish the calls to the mock. There should normally not be one assert with -Exactly -Times 2, there are exceptions of course.
But as @PlagueHO mention, I think normally it is those calls that actually make a change that should have an assert, or if a call should always return a specific information for that code path an assert should be used to, to make sure the correct (expected) information was returned.
It is possible to use Assert-VerifiableMocks for the other information calls that just return information. To make sure no mocks are added that are never called at all.
I personally sometimes also add -Exactly -Times 0 to some asserts where I want to make sure that in that certain code path, for example that a "remove" call is never made.

I'm don't agree that we should remove assert calls. Not sure how we can add this to the guidelines since there are so many edge cases. Suggestions on that?
Also, I'm not sure how we can add improve the templates without them being to complex, thinking on the ParameterFilter. 🤔 They should be relative easy to read for a newcomer.

The problem in xWebSite might be that some code should be made into helper functions, and those helper functions should be mocked instead. Although, the helper functions should be tested as well, but separate so there can be less code in the main unit test for xWebSite. 🤔

[stale]

This issue has been automatically marked as stale because it has not had activity from the community in the last 30 days. It will be closed if no further activity occurs within 10 days. If the issue is labelled with any of the work labels (e.g bug, enhancement, documentation, or tests) then the issue will not auto-close.

[PlagueHO]

I think this discussion should stay open as some guidance would be good. Might also be an opportunity for a decent blog post - when to Mock with parameters and when to not Mock with parameters.

[gaelcolas]

To summarise:

• testing behaviour is preferred over Assert-MockCalled
• Assert-MockCalled is useful to test code path when it's not possible to directly test behaviour
• Assert-MockCalled should be used tactically, not by default, because it makes refactoring harder
• Too many Assert-MockCalled indicates a possible need to refactor, such as in xWebSite

Call to Actions

• Improve the documentation
• Refactor xWebSite

[cezarypiatek]

One more observation:

DO NOT use Assert-MockCalled to verify if module internal method was invoked

[cezarypiatek]

One more issue: there is a lot of mocking of module internal methods. I think only module external methods should be mocked. Mocking module internal methods undermine test credibility.

[cezarypiatek]

Most of the tests currently looks as follows:

1. Prepare Parameters
2. Prepare Mocks for internal & external methods
3. Call  Set-TargetResource
4. Verify if mocks were called

Why not to write test according to this pattern:

1. Prepare Parameters
2. Prepare Mocks for external methods
3. Call  Set-TargetResource
4. Verify if Get-TargetResource returns expected values

[PlagueHO]

@cezarypiatek, I agree in many situations, but definitely not all:

If an internal function changes state of the system then it should be mocked. It is generally a good design pattern to separate lower level abstraction functions (functions that change state) into their own function to make mocking easier and to meet single responsibility principle (SRP/SOLID). So, mocking those sorts of internal functions and asserting that they was called I is acceptable.

An example: When there isn't a built in Cmdlet that performed the desired state change and so an "internal" function needs to be created to perform the work - e.g. "Set-PowerPlan". It is absolutely acceptable to Mock the Set-PowerPlan function when testing Set-TargetResource even though it is an internal function. I want to test Set-TargetResource, not the implementation of Set-PowerPlan.

The key for me is that the tests validate the function and the same level of abstraction. If the function under test is a "logical" layer (e.g. Set-TargetResource) then it should mock all functions calling into the "physical" layer. Tests should validate the behavior of the function under test, and not necessarily the implementation of the "physical" layer as this may be done different ways. Not being able to Mock the physical layer implementation results in brittle tests.

But otherwise I agree with you and I have seen situations (and even written them) where in hind-sight I shouldn't have.