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

Fix mock behavior for Mock and Should -Invoke #1915

Merged
merged 11 commits into from
Apr 23, 2021
Merged

Fix mock behavior for Mock and Should -Invoke #1915

merged 11 commits into from
Apr 23, 2021

Conversation

nohwnd
Copy link
Member

@nohwnd nohwnd commented Apr 22, 2021

Set mocks into the module specified by -ModuleName, or when not present into the current module. This means that in script it will be set into the script, but when running InModuleScope then the module will be used.

Fix how mock resolve is done to respect the -ModuleName, and only fallback to the default script behavior when a parametrized mock in module fails and there is no default behavior.

Remove mocks in all modules and user scope on startup to clean up after cancelled runs.

Throw when we resolve command in Should but it is not a Hook.

Fix #1827
Fix #1878
Fix #1868

@nohwnd
Copy link
Member Author

nohwnd commented Apr 22, 2021

@fflaten this should fix the f calls mock from o, when invoked in script scope. I did not look at the should side yet. Anyways this isFromRequestedModule and the mock resolve in script scope if you can't find function in the requested scope are imho two main sources of Mock confusion. I will revisit what it breaks when we don't resolve in caller scope, and also need to have a look on the Should -Invoke.

@nohwnd nohwnd added this to the 5.2 milestone Apr 22, 2021
@fflaten
Copy link
Collaborator

fflaten commented Apr 22, 2021

Good stuff! At least for me feels more consistent and the gist incl. comment run as expected now.

This also fixes #1878 as the returned callhistory no longer includes mock-behaviors while using contextinfo for the original cmdlet. They will now get Expected Start-Job to be called at least 1 times but was called 0 times as expected when forgetting to provide -ModuleName to Should-Invoke while using a module-scoped mock.

@fflaten
Copy link
Collaborator

fflaten commented Apr 22, 2021

Should these be interchangeable? If so then that broke by this change.

Mock Start-Job -MockWith {
    Write-Warning "Mocked scriptblock"
} -ModuleName "SomeModule"

and

InModuleScope -ModuleName "SomeModule" -Scriptblock {
    Mock Start-Job -MockWith {
        Write-Warning "Mocked scriptblock"
    }
}

Using this PR, calls to mock using the latter method is no longer detected. The mock is invoked as expected, but assertion fails.

# It-block in normal script scope. Only mock was generated using `InModuleScope`
It "Assertion works" {
    # Call function exported from SomeModule that uses Start-Job
    Invoke-SomeAction
    Should -Invoke -CommandName Start-Job -ModuleName "SomeModule" -Times 1 -Scope It
}

@nohwnd
Copy link
Member Author

nohwnd commented Apr 22, 2021

They should be interchangeable I think, imho should Invoke still relies on the module name from module, not the target module name. I also need to rename internally, ModuleName has too many meanings.

@nohwnd
Copy link
Member Author

nohwnd commented Apr 22, 2021

Maybe Should -Invoke could take -ModuleName * as well to ease the pain if we broke some edge cases that would work without module name before.

@fflaten
Copy link
Collaborator

fflaten commented Apr 22, 2021

They should be interchangeable I think, imho should Invoke still relies on the module name from module, not the target module name. I also need to rename internally, ModuleName has too many meanings.

Sounds good. I think a comment in the code to describe the format/logic of the repeating $key = "$ModuleName||$CommandName" would be useful too unless the mentioned rename of $ModuleName will make this more obvious. I've personally found it hard to keep track of what to expect at all times.

@nohwnd nohwnd changed the title Fix behavior resolve to use target module Fix mock behavior for Mock and Should -Invoke Apr 23, 2021
@nohwnd
Copy link
Member Author

nohwnd commented Apr 23, 2021

@fflaten the mock side should now behave very consistently with parametrized behaviors always be specific to the target module (the one specified by -ModuleName) and the default behavior being taken from the target module, or when not present from the script scope, or when not present falling back to the original command.

The logging should be greatly improved.

Should side still as is.

Please run it through your tests and the example to sanity check me. :)

@fflaten
Copy link
Collaborator

fflaten commented Apr 23, 2021

@nohwnd The last commits didn't affect the testruns i did above. Original gist+comment is still fixed 🎉. Mocks created inside InModuleScope without ModuleName in Mock still don't behave the same as Mock -ModuleName . See https://gist.github.com/fflaten/d7529c5ea9cf0dbd668a67155af64299

@nohwnd
Copy link
Member Author

nohwnd commented Apr 23, 2021

Think this is good to go. But I am not sure about the usefulness of the mock fallback when there is no default behavior but there is parameter filter that does not pass. Problem is it won't fallback when we don't have mock in that module. Not sure how to fix it, sounds like global function and alias could help, or -ModuleName * that defines mock in all currently loaded script modules and script scope as well.

@nohwnd nohwnd merged commit 87a3fb6 into v5.0 Apr 23, 2021
@nohwnd nohwnd deleted the module-name-filter branch April 23, 2021 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants