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

Make initial Mock cleanup more performant #2332

Merged
merged 4 commits into from Apr 1, 2023
Merged

Conversation

nohwnd
Copy link
Member

@nohwnd nohwnd commented Mar 30, 2023

PR Summary

There is initial cleanup of orphaned mocks that happens in Invoke-Pester and makes sure that there are no Mock aliases in place coming from previously cancelled runs. In Pester v4 we only cleaned up the main session state, but in v5 we pick up every loaded module and clean up there. This adds up to the point where it can take several seconds (in docker container) to do this cleanup.

This PR optimizes the cleanup to just query functions, rather than all cmdlets and filtering them.

PR Checklist

  • PR has meaningful title
  • Summary describes changes
  • PR is ready to be merged
    • If not, use the arrow next to Create Pull Request to mark it as a draft. PR can be marked Ready for review when it's ready.
  • Tests are added/update (if required) (there are no tests for this functionality in the first place imho)
  • Documentation is updated/added (if required)

@nohwnd nohwnd requested a review from fflaten March 30, 2023 16:28
@nohwnd
Copy link
Member Author

nohwnd commented Mar 30, 2023

It is tested: [n] ERROR: - Invoke-Pester cleans up orphaned mock hooks and aliases in modules ->

will need to fix.

Copy link
Collaborator

@fflaten fflaten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with the idea, but are we able to mute these? Could cause errors in a follow-up run.

> & (Get-Module PSReadLine) { dir function:\ }                                                                                                   
Exception: /workspaces/Pester/src/functions/Pester.SessionState.Mock.ps1:592:9
Line |
 592 |          throw "Mock data are not setup for this scope, what happened? …
     |          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     | Mock data are not setup for this scope, what happened?

Not sure which property being enumerated is causing this.

> & (Get-Module PSReadLine) { Get-Command Pester* } | % parameters
InvalidOperation: 
Line |
  34 |  … icparam { & $MyInvocation.MyCommand.Mock.Get_MockDynamicParameter -Cm …
     |                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     | The expression after '&' in a pipeline element produced an object that was not valid. It must result in a command name, a script block, or a CommandInfo object.

Maybe silenced by checking for Mock-property before invoking it? Even better, are we able to self-delete based on similar condition there and/or begin?

src/functions/Mock.ps1 Outdated Show resolved Hide resolved
@nohwnd
Copy link
Member Author

nohwnd commented Mar 31, 2023

I've reverted the change, and remove the functions as before, but now just querying the item provider for functions rather than asking Get-Command to give me all publc commands (including cmdlets) that are available in all modules. This seems to take 7ms (and 1 ms after warmup), vs 55ms (and 30ms after warmup), and should not depend on the number of modules that are imported.

@nohwnd
Copy link
Member Author

nohwnd commented Mar 31, 2023

@SQLDBAWithABeard could you try this before I merge please?

@nohwnd nohwnd changed the title Don't remove functions in initial Mock cleanup Make initial Mock cleanup more performant Mar 31, 2023
@SQLDBAWithABeard
Copy link

@SQLDBAWithABeard could you try this before I merge please?

Totally removes those calls from the top of my performance list and gives me other things to work on :-)

@nohwnd
Copy link
Member Author

nohwnd commented Apr 1, 2023

Rob reported this comparison of calls to Get-Command -Name PesterMock_* vs Get-ChildItem function:/PesterMock_*

With 8 modules loaded
20 Milliseconds
2 Milliseconds
With 19 Modules loaded
314 Milliseconds
5 Milliseconds
With 21 Modules loaded inc dbatools/dbacheks
220 Milliseconds
8 Milliseconds

Insignificant difference between 1st and following runs

@nohwnd nohwnd merged commit a3d2070 into main Apr 1, 2023
14 checks passed
@nohwnd nohwnd deleted the simplify-mock-cleanup branch April 1, 2023 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants