Removing InModuleScope from Unit tests - Does it make things worse?

[PlagueHO]

I could use a bit of guidance here from the community.

I'm in the process of removing InModuleScope from the Unit tests in xNetworking (with about 20 other resource modules in my back log). I have some concerns:

The problem is that most (every DSC resource - not just xNetworking) unit tests have mocked objects declared at the beginning of the tests (inside the InModuleScope). This is so that if a cmdlets is mocked several times the object doesn't have to be defined each time in the -MockWith { } block.

You can find an example of this here:
https://github.com/PowerShell/xNetworking/blob/dev/Tests/Unit/xNetworkAdapter.Tests.ps1#L26

These Mocked objects are then used within the -MockWith { } block. For example:
https://github.com/PowerShell/xNetworking/blob/dev/Tests/Unit/xNetworkAdapter.Tests.ps1#L80

But, when the InModuleScope is removed, these Mock objects are no longer in a scope that can be accessed from within the -MockWith.

There are two solutions that I can see:

1. Repeat the actual object within each -MockWith { } block. For example:

Mock Get-NetAdapter -MockWith {
    [PSCustomObject] @{
            Name                    = 'Ethernet'
            PhysicalMediaType              = '802.3'
            Status                         = 'Up'
    }
}

1. Define the Mocked object as a global:

$Global:MockNetAdapter = [PSCustomObject] @{
            Name                    = 'Ethernet'
            PhysicalMediaType              = '802.3'
            Status                         = 'Up'
    }
Mock Get-NetAdapter -MockWith { $Global:MockNetAdapter }

Neither of these solutions is particularly good. Especially 1 because some of the resources contain large object mocks (10 or more parameters) repeated many times over the tests. Using globals is better but should also not be used.

I have tried retrieving the variables in the -MockWith by using Get-Variable -Name MockNetAdapter -Scope ... but haven't had any luck.

I agree that the InModuleScope is best to only be used for private functions, but removing it prevents useful design patterns. It seems like we might be exchanging one bad design pattern (overuse of InModuleScope) for even worse ones (code duplication or global properties).

Almost every DSC Resource uses this design pattern (including the HQRM SharePointDsc) and I am beginning to think that removing it will actually cause worse patterns to have to be adopted. @BrianFarnhill - are you planning on removing InModuleScope from all the unit tests for SharePointDsc?

@TravisEz13, do you think you could have a go at removing the InModuleScope { } in https://github.com/PowerShell/xNetworking/blob/dev/Tests/Unit/xNetworkAdapter.Tests.ps1 (I think you wrote these tests) and see if you can come up with some solutions to the problems caused by removing InModuleScope?

@narrieta, @KarolKaczmarek, @kwirkykat, @mbreakey3 - You all have much stronger PowerShell-fu than I do. If you could come up with some design patterns then I'm happy to document them.

Really appreciate the guidance from you all. Cheers!

[PlagueHO]

Tagging @iainbrighton in this because I know he has done some unit tests where InModuleScope was removed (I can't remember which resource though) - so he might have some ideas or suggestions.

[TravisEz13]

@PlagueHO, I think the use of globals would be the only practical option. To completely remove using the in module scope....
The other would be to use the inmodulescope to add the objects, then run the tests outside the module scope.

[BrianFarnhill]

So for SharePointDsc we don't have any plans to remove it - but if we need to then we can explore it for the sake of consistency and getting along. From my understanding though the InModuleScope that basically helps by adding all the mocks to the module scope, right? Isn't that what we want - we want to make sure that we mock the calls within just the scope of that module to ensure that we are seeing the expected behaviour? If we make a change to call them at a global scope or some other level aren't we creating a risk that they "somehow" get called from somewhere else during the test and not where we expect them to?

It was my view (and I wouldn't mind getting the opinion of @nohwnd on this one) that by using the InModuleScope we could contain where the mocks would be injected, helping make sure that they were only mocked inside the calls to our DSC resources.

I suppose I don't see the reason to not use the InModuleScope bit either. Again if you go and have a look through SharePointDsc's unit tests we have made use of it everywhere and we have (without wanting to sound too modest) some pretty decently structured unit tests throughout.

There's my two cents, but like I said happy to be a team player either way on this one.

[PlagueHO]

@TravisEz13 - I might give your suggestion a try (creating the objects for Mock usage) and see if that is a workable solution. If it is then I'll add it to the TestGuidelines.md documentation.

I agree @BrianFarnhill - almost every current DSC resource unit test I've ever seen uses InModuleScope in the way you describe. I know every unit test I've ever written adopts this practice because it has s many benefits. I took a quick look at the SharePointDsc unit tests and the amount of work removing the InModuleScope from the public functions (*-TargetResource) would be huge.

I've done quite a bit of work on xNetworking attempting to remove the InModuleScope around the public functions and it is as @BrianFarnhill says: Almost every DSC resource unit test uses InModuleScope to allow the mocks to be created in the context of the module being tested. This can be worked around by adding a -modulename parameter to the Mock and Assert-MockCalled functions.

I'm definitely OK with removing the InModuleScope usage from around all public functions if that the community decides that is the standard practice. But I think if that is the case then we need to figure out the following:

1. How to handle repeated variables and objects used in Mocks (as I queried earlier)
2. What should we do with existing DSC Resources that use InModuleScope around public functions - do we leave them or update them?
3. Does the use of InModuleScope around public functions in unit tests preclude them from being HQRM?

What I'd suggest is we have an attempt at converting a single unit test to remove InModuleScope around the public functions and actually identify all the problems and downsides to doing this. If the downsides are acceptable then we could use this conversion to document the process as well as add the examples to the TestGuidelines.md.

Because of technical debt that will be added to all DSC Resources because of this change I think that it should be planned and documented carefully.

Just my 2c.

[nohwnd]

You need to use InModuleScope in two cases: Testing internal module stuff, and testing functions that come from modules imported inside of your module (let's call them "incoming functions").

You probably should not use InModuleScope for integration testing, because there you usually want to test the public (exported) part of your module. You can still use InModuleScope to mock the incoming functions by putting only the mocks in the module scope, and keeping the rest of the code outside of the module scope.

I say "probably" because a module function runs in it's module scope no matter if it's exported or not. Exporting is just a way to limit what is visible outside of the module. Running all the test code in the module scope might give you false positives, because you accessed something that ends up being hidden for the end user, but if you are careful then not much happens.

In cases where you don't need to use InModuleScope it's really up to you. I would not encourage you to use it everywhere, but I would not consider it a bad practice either, as long as everybody is aware of the implications. Personally I try to use it only when I need it, because it adds another level of indentation and I don't like looking inside of modules unless I have to.

For your codebase, I don't see any reason to invest a lot of time and effort to remove InModuleScope from your existing test code, not to mention that you'd risk breaking it. Whether you enforce usage of InModuleScope everywhere in the unit tests or not is up to you, just make sure everybody is aware that they are looking at the internal representation of the module and that the public surface is different.

@PlagueHO

1. How to handle repeated variables and objects used in Mocks (as I queried earlier)
   Put your mock inside of a function. That way you can access it from everywhere.
2. What should we do with existing DSC Resources that use InModuleScope around public functions - do we leave them or update them?
   Keep it. Both exported and internal functions still run inside of the module scope. The InModuleScope just enables your test code to see more of the internals. In most cases I don't see any reason to distinguish between exported and internal functions in your test code. When you validate what you exported you'd do it by looking at the module metadata, and not by Get-Command.
3. Does the use of InModuleScope around public functions in unit tests preclude them from being HQRM?
   As I see it doesn't. The guidelines say that you don't need to use InModuleScope not that you must not use it.

#content of amodule.psm1
#Import-Module NetAdapter
#function abc {
#    Get-NetAdapter
#}
#

function Mock-NetworkInterface 
{ 
    Mock Get-NetAdapter -MockWith {
        [PSCustomObject] @{
            Name              = 'Ethernet_test'
            PhysicalMediaType = '802.3'
            Status            = 'Up'
        }
    }
}

get-module NetAdapter | Remove-Module
get-module amodule | remove-module
Import-Module  -Name "C:\Temp\amodule.psm1" -Force

InModuleScope -ModuleName amodule {
    Describe "mocking and testing inside of module scope" {
        It "mocks"{
            Mock-NetworkInterface            
             abc | select -ExpandProperty Name | Should Be 'Ethernet_test'
        }
    }
}


get-module NetAdapter | Remove-Module
get-module amodule | remove-module
Import-Module  -Name "C:\Temp\amodule.psm1" -Force

Describe "mocking inside of module scope testing from the outside" {
    InModuleScope -ModuleName amodule {
       Mock-NetworkInterface
    }

    It "mocks"{
        abc | select -ExpandProperty Name | Should Be 'Ethernet_test'
    }

    It "does not mock"{
        Get-NetAdapter | select -ExpandProperty Name | Should Not Be 'Ethernet_test'
    }
}

[PlagueHO]

@nohwnd - that is some really great info. Thank you!

The example is extremely useful. I'll have a go at converting the xNetworking unit tests over to this method. If everything goes OK, I'll add the examples and documentation to the TestGuidelines.md.

[nohwnd]

You are welcome. One more trick that may come handy when you debug. To peek inside of a module, without all the InModuleScope fanciness, do this:

& (Get-Module ModuleName) { code to run inside of the module }

#so for example looking at the mock table 
#of pester while you are on a breakpoint in the middle of an 'It'
& (Get-module pester) { $script:mockTable }

[gordonbondon]

It is possible that scoping problem will be fixed in v4 of Pester. See this: pester/Pester#204

[johlju]

I tried to use this method in my test. My goal is to use local or script variables inside the mock. But when trying to call the function that holds the Mock (Get-MockSQLAlwaysOnAvailabilityGroupListener). It can't find it.

@nohwnd If you would be so kind to see if I'm doing something wrong in my code? Starring me blind if I'm missing something from your example.

I uploaded a small test here (still to big to paste in); https://github.com/johlju/Samples/blob/master/Tests/Dummy.Tests.ps1

The error I get is:

Describing xSQLServerAvailabilityGroupListener\Get-TargetResource
 [-] Error occurred in Describe block 92ms
   CommandNotFoundException: The term 'Get-MockSQLAlwaysOnAvailabilityGroupListener' 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>, ...\Tests\Unit\Dummy.Test.ps1: line 72

[johlju]

Oh, and @nohwnd. I must point out that the "Dummy" script I linked to above, it does work in PowerShell ISE. But it does not work in PowerShell prompt, and subsequently not in AppVeyor.

[PlagueHO]

Putting some work into xNetworking unit tests at the moment and using the great info everyone has provided here. Once I'm happy everything is complete I'll submit the info to this repo TestGuidelines.md.

[TravisEz13]

@PlagueHO Thanks

[PlagueHO]

Unfortunately I haven't made any progress on this. The suggested solutions all resulted in a significant complexity increase and often I couldn't make them work at all (I agree, they should work, but for various reason they don't).

As per the community call today I'd like to wait till Pester has a better solution for this before doing any further work on this one if that is OK with everyone?

[mbreakey3]

As of now, we've decide to leave 'In Module Scope' in our templates. If you encounter any issues with it feel free to file an issue or to use some of the suggested workarounds.