On-demand SetupAllProperties

[ishimko]

It is implementation of lazy SetupAllProperties, as described in #740 by @stakx.

New switch AutoSetupProperties is added, current SetupAllProperties implementation is slightly hacked to behave the same as it was before. Eager setup is moved to interceptor to setup properties being accessed on-demand.

New code passed all existing tests and I've added some more based on comments in the existing code about corner cases.

As the word "switch" was mentioned in the discussion, I decided to go via existing Switches enum. What bothers me is that now there are two ways of doing almost the same: SetupAllProperties call as before and enabling AutoSetupProperties switch directly. SetupAllProperties is hacked to behave like eager version before (by saving DefaultValueProvider) and switch doesn't have this (I just decided so, but we can actually save DefaultValueProvider in the setter of Switches...). It looks logically to me, that switch does not save DefaultValueProvider, since switch is more declarative, while SetupAllProperties is imperative (even though it is implemented in a new way).

What do you think?

[stakx]

Hi @vanashimko, I likely won't have time to review your PR until sometime in June due to being very busy this month, but I'll eventually get to it. I'd like to ask you to be patient, though.

It would've been good to discuss this in an issue first, since there are some finer (and AFAIK undocumented) points you might not yet be aware of which, if we're unlucky, make the whole idea of a lazy SetupAllProperties non-feasible (we might currently be lacking unit tests for that). I'm mainly thinking of correct choice of method overloads + setup order here... it's a pain point in Moq that hasn't been solved cleanly yet, and it was a very central issue the last time I had to touch SetupAllProperties.

That being said, judging from your description, you definitely seem to be on the correct track here. Ideally, SetupAllProperties should do nothing more than enable the switch. Using Switches to house that new switch is sensible (although I'd hoped to deprecate Switches in the near future). Adding a new step in the interception pipeline (before Return) is what I'd have done, too.

I'll try to review in detail soon.

[ishimko]

Thanks for you response, @stakx!

Even if you'll find that due to some "finer points" lazy SetupAllProperties is non-feasible, it was a nice time hacking around the codebase...

[stakx]

To give some more context for the ominous "finer points" (should you choose to look into it):

You might have noticed code that gathers all properties in topological ("depth-first") order. I only took a quick look at your first commit and saw that you removed this logic (and I'm glad you did, as it's part of why SetupAllProperties has bad performance). In the absence of that code, it's now important to ensure that more derived properties are still favoured over less derived ones when choosing which one to set up (e.g. for the case where a derived class makes a read-only base property writable, too; and there may be more cases such as properties from a reimplemented interface). When property stubbing becomes lazy, I'm not sure this is still guaranteed as the order in which you access properties might now start influencing things and we have no control over that.

I suspect that things still work (i.e. all tests pass) despite the removal of the topological search due to the recently introduced GetReboundProperty helper method, but that's just a suspicion; we should take a closer look at this to ensure we don't break anything.

(In fact, this problem is not specific to just properties: What Moq is still lacking is a clearly defined algorithm that maps between invoked methods and declared methods (setups). That algorithm needs to work in a variety of cases: interface methods implemented by a class, methods of reimplemented interfaces (mock.As<T> where the mocked type is a class that explicitly implements T), class methods, overridden class methods, etc. We have an algorithm that works "well enough" but it's hard to understand because it's grown from experience instead of having been methodically derived from the CLI standard and from the way DynamicProxy maps methods to IInvocation.Method.)

[ishimko]

Thanks, I'll try to get a closer look and add more tests.

My thoughts were that if we already have calling method (which represents accessing property) in Invocation passed to IInterceptor, we should not bother much about the order of traversal of hierarchy. The were some stuff related to base interfaces (GetProperty surprisingly does not return property of base interfaces if called on an interface) and reimplemented interfaces via As, but there are tests for these cases, so I catched this at early stage.

If there are some nifty moments with the method we have in Invocation.Method, so maybe my optimistic assumption was wrong...

[stakx]

@vanashimko, thanks for waiting, and for this PR. This looks like really solid work 💪 and I am definitely in favour of merging it! Below you'll find a few review comments on things that we might be able to refine or simplify a little further.

Once again, thanks for the great work! 👍

[ishimko]

@stakx, thanks for the so detailed review! I'll be looking into the comments.

[stakx]

Awesome work! ❤️ I think this is ready for merging.

If you want to clean up / polish your commits any more (but that's totally optional, feel free to leave it as is), now would be a good time. I'll wait a little longer before merging.

Also, in case that you want your contribution to be more publicly acknowledged, you could add a new changelog entry, e.g.:

 The format is loosely based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).


+## Unreleased
+
+#### Changed
+
+* Improved performance for `Mock.Of<T>` and `SetupAllProperties` as the latter now performs property setups just-in-time, instead of as an ahead-of-time batch operation. (@vanashimko, #826)
+
+
 ## 4.11.0 (2019-05-28)

[ishimko]

Thanks @stakx!

I've added changelog entry as you suggested. I'm OK with squashing all my commits into one automatically by GitHub during merge.

[stakx]

Just one more baby change request for the changelog.

[stakx]

🚀

[stakx]

Thanks again so much for this contribution, it's been a pleasure to review.

(Please feel free to contribute more, if you are interested and can spare the time. #740 and #835 might be good candidates now.)