-
-
Notifications
You must be signed in to change notification settings - Fork 805
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 for slow down with many setups on single mock #1111
Conversation
…a duplicated expectation is added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @CeesKaas, thanks for your patience.
I do agree that we should do something about this problem. I've thought about it for a bit, and your solution indeed seems to be the easiest.
Just a couple of details that need your attention. Then this should be good to merge!
Co-authored-by: stakx <[email protected]>
This reverts commit d90d3b1.
@@ -101,6 +105,7 @@ public void Clear() | |||
lock (this.setups) | |||
{ | |||
this.setups.Clear(); | |||
this.activeInvocationShapes.Clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a remark for posteriority: in theory, we should also filter out removed setups in RemoveAllPropertyAccessorSetups
. Not doing means that the first time a property setup is recreated after a call to mock.SetupAllProperties()
, there may be a superfluous call to MarkOverriddenSetups
. This is an edge case however, and the overhead of the additional MarkOverriddenSetups
in this case is probably quite small, so let's just ignore this for now.
All good. Thanks for contributing, @CeesKaas! 🚀 |
I've played with the code a bit this morning and this change fixes the unittests I've added (at the cost of a bit of extra memory consumption and an extra operation always to save n operations in most cases)
fixes #1110