Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow InModuleScope to not require the module to be loaded during discovery #1885

Closed
JustinGrote opened this issue Mar 25, 2021 · 5 comments
Closed
Labels

Comments

@JustinGrote
Copy link
Contributor

General summary of the issue

There are times when you only want the discovery phase (for instance, Tyler's Pester Text Explorer Extension) to determine the test structure. However, currently if you want to use InModuleScope, the module must be loaded during discovery, producing extra overhead especially in repeated test discovery scenarios. I don't see a strict reason this is necessary, and the check for module should be done later.

Describe your environment

Pester version     : 5.1.1 C:\Users\JGrote\OneDrive - Allied Digital Services LLC\Documents\PowerShell\Modules\Pester\5.1.1\Pester.psm1
PowerShell version : 7.2.0-preview.4
OS version         : Microsoft Windows NT 10.0.19043.0

Steps to reproduce

    $mymodulename = Get-MyDynamicModuleName
    InModuleScope $mymoduleName {
      ...
    }

Expected Behavior

Module would not be tested if it is loaded until the actual test phase

Current Behavior

Pester fails at the discovery phase

ystem.Management.Automation.RuntimeException: No modules named 'mymagicmodule' are currently loaded. 
at Get-ScriptModule, C:\Users\JGrote\OneDrive - Allied Digital Services LLC\Documents\PowerShell\Modules\Pester\5.1.1\Pester.psm1: line 8964
at InModuleScope, C:\Users\JGrote\OneDrive - Allied Digital Services LLC\Documents\PowerShell\Modules\Pester\5.1.1\Pester.psm1: line 8927at <ScriptBlock>, C:\Users\JGrote\Projects\Press\Source\Public\Get-Version.Tests.ps1: line 2
at <ScriptBlock>, C:\Users\JGrote\OneDrive - Allied Digital Services LLC\Documents\PowerShell\Modules\Pester\5.1.1\Pester.psm1: line 2903at Invoke-File, C:\Users\JGrote\OneDrive - Allied Digital Services LLC\Documents\PowerShell\Modules\Pester\5.1.1\Pester.psm1: line 2912  
at Invoke-BlockContainer, C:\Users\JGrote\OneDrive - Allied Digital Services LLC\Documents\PowerShell\Modules\Pester\5.1.1\Pester.psm1: line 2837
at Discover-Test, C:\Users\JGrote\OneDrive - Allied Digital Services LLC\Documents\PowerShell\Modules\Pester\5.1.1\Pester.psm1: line 1411at Invoke-Test, C:\Users\JGrote\OneDrive - Allied Digital Services LLC\Documents\PowerShell\Modules\Pester\5.1.1\Pester.psm1: line 2356  
at Invoke-Pester<End>, C:\Users\JGrote\OneDrive - Allied Digital Services LLC\Documents\PowerShell\Modules\Pester\5.1.1\Pester.psm1: line 4839
at <ScriptBlock>, <No file>: line 1

Possible Solution? (optional)

Don't check the module is loaded until testing phase.

@JustinGrote JustinGrote changed the title Allow InModuleScope to not require the module to be loaded during discovery [Feature Request] Allow InModuleScope to not require the module to be loaded during discovery Mar 25, 2021
@nohwnd nohwnd added the Feature label Mar 29, 2021
@nohwnd nohwnd changed the title [Feature Request] Allow InModuleScope to not require the module to be loaded during discovery Allow InModuleScope to not require the module to be loaded during discovery Mar 29, 2021
@nohwnd
Copy link
Member

nohwnd commented Mar 29, 2021

@JustinGrote I had the same idea, but it is quite complicated and the way InModuleScope works would have to change a lot. Leaving some edge cases I am not sure how to solve.

Right now it uses the fact that scriptblock is tied to the session state that created it. So InModuleScope loads the module and then simply runs all the code inside of that module. This ties all the Describe and It, and all other stuff running in Discovery to the module, including TestCases.

In effect all the scriptblocks that are attached to It block will run in the module scope on Run, because during discovery they were created inside of the module.

If we were to avoid loading the module, we would have to take a note of the module name during Discovery, and then modify every scriptblock provided, to run inside of the module during Run. This still has at least three problems:

  • Describe - the scriptblock assigned to Describe runs during discovery, and it would be attached to the script scope, this is not a problem as long as all the code in it is either evaluated during run, and there is no extra code in the body of the Describe

  • testcases - TestCases (-ForEach) run in the scope of Describe and are evaluated during Discovery, we would need to have lazy testcases that would evaluate in Run, that is provide them as a scriptblock that produces array of hashtables instead of an array of hashtabled, and then bind it during run to the correct session state. This also allows the variables from BeforeAll to be easily used in the testcases, but makes filtering more complicated, and changes the counts, because not all tests are created during discovery. But it should be possible.

  • Skip - skip is often used as -skip:(), this code also evaluated during discovery and could not be tied to the internals of the module. This seems like an okay compromise, and could be done as -SkipLazy { } which would again eval on run, we skip tests during Run anyway, and not in Discovery. Here the limitation is more "technical" because I would love to have the ability to provide scriptblock to the original -Skip parameter, but switch cannot be extended to do that.

  • Tags, and all other filters - here the consideration is only in regards to the possibility of having -Filter { some arbitrary filter }, because filtering needs to be done during discovery, and wo se would not have the Data (result of TestCases) available at that time.

  • We need to consider existing usages of InModuleScope for which this would be a breaking change, and I don't really like that cmdlet anyway. IMHO a better way to implement this would be adding -ModuleName on all block (e.g. Describe and It), and have that lookup the module and bind it on Run.


A completely different option is that we avoid the problem that caused not recommending InModuleScope outside of It in the first place, which was running tests after the whole discovery is done. This makes the scriptblocks to be bound to module session state that might no longer be the same (if you force import the module in some later discovery step) so your test and mocks will each run in a different session state, which come from the same module.

Running discovery right before Run seems to be nice for parallel execution because that will always be split by file (otherwise there are some other serious problems). But the problem is that you don't know which describe is the last in the file, so you don't know when to execute Run in the file. Solving this problem would solve a lot of other problems. But before you suggest AST, then we can't really use that because Describe can be wrapped in whatever other function.

@JustinGrote
Copy link
Contributor Author

@nohwnd Thanks for the notes. I can say the other place i use InModuleScope to mock commands in the module scope as the mocks don't work "normally".

Example:

    It 'Fails gracefully if Git is not present' {
        InModuleScope Press {
            Mock Get-Command -ParameterFilter { $Name -eq 'git' } { $null }
        }
        { Get-Version -ProjectPath $TestDrive } | Should -Throw 'git was not found*'
    }

@nohwnd
Copy link
Member

nohwnd commented Mar 30, 2021

What if you do Mock -ModuleName Press ?

@JustinGrote
Copy link
Contributor Author

What if you do Mock -ModuleName Press ?

I slap my hand to my forehead saying "I've done that before and for some reason forgot about it"

@JustinGrote
Copy link
Contributor Author

Using -ModuleName parameter or simply testing by dot sourcing the functions directly instead of trying to do it inside module scope was my resolution for this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants