Add extensibility point for custom auto providers #259

zvirja · 2016-11-08T14:10:05Z

This PR creates extensibility point for external parties to specify custom default values. This is a merge of #156 and #234.

Changes overview:

Added custom handlers call action to RecordReplay route.
I haven't created any extension method for this feature. That is because I assume this feature to be used rarely and extension method will pollute the completion menu. Moreover, given that very low-level API is provided, it doesn't seem to be a feature for "broad masses" :) Let me know if you have different opinion.

For now, API is following: SubstitutionContext.Current.GetCallRouterFor(source).RegisterCustomCallHandler(factory).
Introduced OriginalArguments to the ICall interface and use them for specifications. This way if call handler (or user in When/Do statement) modify argument values (e.g. for ref parameters), that doesn't affect rules. Check the following tests: test1, test2.
AutoValue feature uses its own cache for known results, rather than register its result globally. Current behavior leads to "increased priority" for auto-values, if setup is done after the first invocation. See this test for more details. Also, see this discussion.

alexandrnikitin

Alex, Thank you for the PR! I left few comments after quick review. But I need to dig deeper into this.

alexandrnikitin · 2016-11-08T14:32:08Z

Source/NSubstitute.Acceptance.Specs/CustomHandlersSpecs.cs

+        }
+
+        [Test]
+        public void First_added_handler_has_precedence()


I think it's better to be consistent with the existing API e.g. Returns - last specification overrides previous. @dtchepak WDYT?

It might be not very intuitive if we reverse order. That is not a regular "Returns", rather that is a pipeline member. I'd expect that registered handlers are executed one by one, rather than in opposite order.

But if you feel that it might be confusing - NP, I'll change that.

Hmm, I don't have strong opinion here. If we treat it as a pipeline then yes, "first handler wins" sounds reasonable.

You are right. If we're dealing with handlers then it's better to traverse from first handler to last added one.

alexandrnikitin · 2016-11-08T14:39:41Z

Source/NSubstitute.Acceptance.Specs/CustomHandlersSpecs.cs

+                    }));
+
+            //act
+            source.MethodWithArgs(Arg.Any<string>(), Arg.Is("42"));


Missed "specification" part ie Returns("")

Yep! Added.

alexandrnikitin · 2016-11-08T14:43:45Z

Source/NSubstitute/Core/CallResults.cs

        ConcurrentQueue<ResultForCallSpec> _results;

-        public CallResults(ICallInfoFactory callInfoFactory)
+        public CallResults(ICallInfoFactory callInfoFactory, bool skipVoidMethodsForPerformance)


I'm not sure that we need skipVoidMethodsForPerformance. How will it be used?

I added it to make this code reusable from AutoFixture side. I'll need my own cache for already resolved results, so CallResults (more precisely, CallResultsCache wrapper) is ideal for that. However, AutoFixture sets results for the ref and out parameters, while method might be void. CallResults will just ignore such methods :(

I've decided to not change this behavior by default, because that could lead to performance degradation.

I would remove this "hack" and implement that logic on AutoFixture side. Is it possible?

Of course, I can do that, just don't like idea to copy-paste the whole class to change a single place. If later you fix some bug in this class, AF will not be updated :(

What if I rename ReturnsVoidFrom method to bool SkipVoidMethod() and make it protected virtual? Will you accept such idea? 😊

Changed API. See in new commits. Do you still have concerns about how it looks?

(Not related to this one but) In any case, code duplication is much better than the leaky conventional abstraction that just few people know about 😉

I still have some confusion regarding the "by design" CallResults usage. I see your point (it's for returning value only), however I'd consider the following sample as fully valid usage:

mock.MethodWithOut(out spec).Returns(c => { c[0] = "outParamValue"; return true; }

Therefore, for me it looks that usage of CallResults (internally) for setting ref and out parameters if you have return value is correct, while if you don't have return value - it's wrong. I'd say such agreement is a bit weird.

In any case, I appreciate your time, your comment and I understand that you are a custodian of this project and it's your duty to keep the code clear. Therefore, I've merged your PR. After all, that is very tiny class, so it should be easy to replicate it without fear :)

Yes, that's perfectly valid usage and Returns API allows it. But you cannot call it for void method, only WhenDo and CallActions work for them.
We will come up to the best solution I'm sure 😉

PS I want to take a part in the NSub-AutoFixture development, please let me know.

But you cannot call it for void method, only WhenDo and CallActions work for them.

Yep, I noticed that. For me it looked like just an API limitation, but it seems to be a part of god's plan :) Deeply inside I still don't agree with it and for me there is no difference (out is essentially the same return, introduced to pass more than one value), but I understand your point and accept it 😯.

I'll mention you and will ask for your review when I implement integration. Let me know if I misunderstood you. Hope, that is not because you noticed that I'm going to make some horrible decisions (though, likely, that is an exact reason given all the discussion above) 😞 😉

Deeply inside I still don't agree with it and for me there is no difference (out is essentially the same return

I agree with you on that. Yes, out is a "return". NSub's API respects that. But it's internals were designed in a different way. And I really don't want to leak that exception tiny via protected method to a 3rd party library.

I'll mention you and will ask for your review when I implement integration.
Yes please. That's not about horrible decisions 😆

alexandrnikitin · 2016-11-08T14:47:02Z

Source/NSubstitute/Core/CustomHandlers.cs

+        private readonly List<ICallHandler> _handlers = new List<ICallHandler>();
+
+        public IEnumerable<ICallHandler> Handlers => _handlers;
+        public IDictionary<object, object> HandlerDataStorage { get; } = new ConcurrentDictionary<object, object>();


I think it's better to stick to C# 5.0 and initialize the property from the constructor.

Is that a strong requirement for the whole project? I noticed that both your CI handled that well. That is a compiler sugar, so produced binary is compatible with .NET 3.5.

I'll gladly change that (and a few other places where I used get-only properties). However, I am just curious about reasons to not use C# 6 features.

No, it's not a strong requirement, it's just my preference to stick to one style. I think I just missed the point when we all switched to C# 6 😊 I call @dtchepak here

I don't have strong feelings about this either way. Consistency is good, progress is good. ¯_(ツ)_/¯ :)

@alexandrnikitin So, it's up to you for now. I'm fine with C# 6 features :)

alexandrnikitin · 2016-11-08T14:57:06Z

Source/NSubstitute/Core/ICallResultsCache.cs

@@ -0,0 +1,9 @@
+namespace NSubstitute.Core
+{
+    public interface ICallResultsCache


What's the reason for introducing ICallResultsCache? It's the same as ICallResults but without Clear() and it just wraps the implementation of CallResults.

Ah I see:

AutoValue feature uses its own cache for known results, rather than register its result globally. Current behavior leads to "increased priority" for auto-values, if setup is done after the first invocation. See this test for more details. Also, see this discussion.

There's enum value AutoValueBehaviour.UseValueForSubsequentCalls that we explicitly use when we specify call for some reason 😕

Yes, that is just a tiny wrapper on top of ICallResults and ICallSpecificationFactory. Rather than have 2 dependencies each time, I decided to unite them to logical unit. It could happen that such kind of objects is required in other places as well (e.g. in ReturnFromAndConfigureDynamicCall), so passing 2 dependencies will pollute consumers.

Do you want me to remove it and use both ICallResults and ICallSpecificationFactory separately?

I think it's better to remove it as that wrapper and its purpose isn't obvious. What do you think?

Well, I see it's purpose - to be a standalone registry to track "known" call results. It has both Add and Get API. To be fair, it isn't just a pure wrapper - it has some logic which combines two APIs.

But given that we can easily substitute ICallResultsCache by (ICallResults, ICallSpecificationFactory) pair - I wouldn't persist too much and will agree to remove it if you feel it's better for further support and such instances just add overhead.

I created a PR to address the ICallResultsCache. Please take a look. https://github.com/Zvirja/NSubstitute/pull/1/files

Merged. Actually yes, you were right, wrapper it too tiny so we could live without it :)

alexandrnikitin · 2016-11-08T14:58:26Z

Source/NSubstitute/Core/ICustomHandlers.cs

+    public interface ICustomHandlers
+    {
+        IEnumerable<ICallHandler> Handlers { get; }
+        IDictionary<object, object> HandlerDataStorage { get; }


What's the purpose behind HandlerDataStorage? Do you think it's needed?

Current implementation is that each CallHandler is stateless and substitute state is present in the state object. Handlers are recreated each time the route is composed.
My thoughts were to have place in state where each call handler could store it's permanent data per substitute (e.g. cached results). Later it could read it from storage when call is dispatched. Alternatively. each handler will need to track it's own dictionary to map substitute to state. That is much harder (and costs in term of memory/performance) than to have a place inside state where it could put its data.

Later I changed implementation and now single instance of custom handler is created per substitute. However, that is rather a "weak agreement" that we will not create handler more than once per substitute.

It looks premature to me. Will it be used in AutoFixture? If no then I think it's better to remove it.

Well, yes, AutoFixture will store its "known" results there - each subsequent call to same method should return the same result. That results are per-substitute, so it's an ideal place to store them.

Do you see any other place where AF can store cached results for substitute calls? I don't see, unfortunately :( Keeping a custom map (subsitute object to AF state) looks a horrible idea.

Another way is to guarantee that we always create a single instance of CallHandler per substitute. But is that OK to make such guarantee a part of CallHandler design? That would be the only ICallHandler we make such guarantee for..

I think I'd prefer to have each custom call handler manage its own state if that's feasible.

AutoFixture details aside, I was imagining we could do something like sub.GetCallRouter().RegisterHandler(new MyHandler(myState)), so a handler would automatically be associated with a single substitute. Don't know if this is practical, but was my initial line of thinking about this.

@dtchepak Many thanks for your example. That is was I was missing. Now it looks obvious, but when I designed API - I didn't see that 😕
Indeed, we could use closure in factory method to associate context with single substitute:

var afState = new AFState(); sub.GetCallRouter().RegisterHandler(subState => new AFHandler(subState, afState));

@alexandrnikitin I'd remove this storage a bit later, it isn't required now.

alexandrnikitin · 2016-11-08T15:09:12Z

Source/NSubstitute/Core/CallSpecificationFactory.cs

@@ -15,7 +15,7 @@ public ICallSpecification CreateFrom(ICall call, MatchArgs matchArgs)
        {
            var methodInfo = call.GetMethodInfo();
            var argumentSpecs = call.GetArgumentSpecifications();
-            var arguments = call.GetArguments();
+            var arguments = call.GetOriginalArguments();


This lines scares me. I have to think about consequences of that change 😟

That's the necessary change, unfortunately. Otherwise, no way for user to configure results using the Returns() extension if my handler fills the ref arguments.
If you see any issues with that - let me know. So far no tests failed :)

@dtchepak While Alexandr is checking that, do you see from your side any problems that could arise because of this change? The idea is that we always use original call arguments for specifications, rather than potentially modified args. So if user changes any argument value (e.g. in When/Do handler), that would not affect call specification.

Hi @zvirja, can you show an example test that will fail if this line stayed as call.GetArguments()?

@dtchepak Sure, check this and this ones.

@zvirja This has always been a problem with out/ref in cases where we set the values from within a Returns. Does your changes fix that problem overall, or just for the custom handlers? (I can check it myself, but thought you might have the info at hand :) )

It changes behavior for all cases, including Returns and When/Do handlers as well - it doesn't matter much whether you modify args via ICall directly or CallInfo.
I've created a bug in AF in past which describes this case (currently AF works via Returns mechanism).

@alexandrnikitin @dtchepak After I investigated that more, I realized that already applied changes are not enough to fully cover this sub-feature. Thank you for pointing me to this direction. If we use original arguments for specification composition, we should also ensure that we take original arguments for matching the calls against the specification.

Therefore, I was forced to add this commit. Please see the added tests - all them fail without the introduced changes.

Sorry Alexandr, now you will need to think about even more set of changes 😉 But we definitely need them. Current behavior of NSubstitute is weak in that area.

P.S. I like how this PR evolves and becomes more mature and consistent.

zvirja · 2016-11-08T16:12:40Z

Thank you very much for your time and feedback! I've put my replies and will continue to track each "local conversation" :)

zvirja · 2016-11-11T14:43:14Z

@alexandrnikitin @dtchepak Did you have any chance to review my answers? I'm a bit confused - you answered me very quickly, I replied and you stopped to reply.. Is everything fine? :) If you just don't have free time - fine, I'll wait.
Just want to ensure that PR is not in a "zombie state" and will be merged before release.

alexandrnikitin · 2016-11-11T15:05:34Z

@zvirja I'm so sorry. It's just a matter of time (read as will/motivation) from my side. I added some comments. I still need time to wrap my head around GetOriginalArguments because I forgot some aspects of NSub 😊

zvirja · 2016-11-11T15:23:10Z

@alexandrnikitin Many thanks for reply. Got it - will just wait in future till you feel enough desire to review all this mess 😉

dtchepak · 2016-11-12T12:57:32Z

@zvirja, thanks for this work and for your patience with us. Just for some context @alexandrnikitin has been out of .net for a while so it really is a wonderful thing he still makes time to help out NSubstitute at all 👍 😄 .

Alexandr, feel free to merge this when you're happy with it, or if you run out of time to allocate to this let me know and I'll pick it up as soon as I can. (I'll run it over a few big test suites I have to reduce the chances of undetected breaking changes.)

alexandrnikitin · 2016-11-15T18:04:41Z

I second David, Thank you for your work and patience, Alex. This would not have happened without you.

Everything looks good to me. I'm ready to merge it. Alex, could you please squash it by yourself. (GitHub UI feature puts the latest author there I think, I want it to be you not me)

I didn't want to sound rude when I wrote about time and motivation. One always have a choice whether it's a family or a couch :)

More precise set of changes: - Ensure that specification evaluates original argument values, because they could be modified during call dispatch. - Ensure that original arguments are used to check whether current call meets particular specification. - Use own results cache for AutoValues route handler. That is needed to ensure that AutoValues priority isn't increased. Otherwise, once handler invoked for the first time, it's "memory" will have same priority as user configured results. As outcome, custom handlers, returns for type, etc will not be invoked.

zvirja · 2016-11-15T20:51:01Z

@alexandrnikitin Squashed, thanks for that hint. Now there is a chance for me to get a "Contributor" badge for NSubstitute project (I believe GitHub looks at commit history) 😸

Thank you guys for your collaboration. I was really impressed by how many resources you are ready to invest to each individual PR despite that you are busy with other tasks. Such experience really inspires to fire another PRs if needed.

Will be looking forward to @dtchepak checks and if there are some troubles - let's discuss that.

dtchepak · 2016-11-17T06:03:19Z

Thanks so much for all this work @zvirja and @alexandrnikitin! 👍 👏

Acknowledge work done in #234, #259, and #156.

zvirja · 2016-11-17T10:50:36Z

@dtchepak Do you have any approximate ETA's when this change will be released, so I can pull it from nuget and start working on feature in AutoFixture project? :)

dtchepak · 2016-11-17T11:51:01Z

@zvirja I hope to get it done in the next few days. Please hassle me if I don't. ;) (I want to check #261 is ok first)

alexandrnikitin · 2016-11-17T12:41:39Z

@zvirja you don't need to wait for the release. You can use NuGet local feeds. Just build the NSub package using build.bat NuGet, put it in the local feed folder, add the feed source and that's it.

zvirja · 2016-11-22T16:41:21Z

I'm unable to build it locally. I've tried to do that on 2 different PCs, but each time FAKE fails with the same error:

System.Exception: Could not run "git describe --tags --long".
Error: Start of process git.exe failed. The system cannot find the file specified
   at Fake.Git.CommandHelper.runSimpleGitCommand@89-2.Invoke(String message) in C:\code\fake\src\app\FakeLib\Git\CommandHelper.fs:line 89
   at Fake.Git.CommandHelper.runSimpleGitCommand(String repositoryDir, String command) in C:\code\fake\src\app\FakeLib\Git\CommandHelper.fs:line 89
   at FSI_0005.Build.getVersion() in C:\External Projects\NSubstitute_Contrib\NSubstitute\build.fsx:line 53
   at <StartupCode$FSI_0005>.$FSI_0005_Build$fsx.main@() in C:\External Projects\NSubstitute_Contrib\NSubstitute\build.fsx:line 60
Stopped due to error

Of course, git is installed and added to PATH 😞

Do you know why that happens and how to overcome it? I've tried to google the solution, but no luck so far :(

That's why I'm still looking forward to @dtchepak release :)

alexandrnikitin · 2016-11-22T17:11:21Z

@zvirja What if you run update-fake.bat and then build.bat?

zvirja · 2016-11-22T17:22:43Z

>__> That helps. That is something I had to try myself 😖
Now there are a lot of dirty files in git, but that is not an issue more :)

P.S. Not it started to work even after I reverted changes and triggered git clean. Something really strange goes here 😫

alexandrnikitin reviewed Nov 8, 2016

View reviewed changes

mrinaldi mentioned this pull request Nov 11, 2016

Generic methods in AutoNSubstitute AutoFixture/AutoFixture#720

Closed

zvirja force-pushed the add-extensibility-point-for-custom-auto-providers branch from 48e11d7 to 26994b8 Compare November 14, 2016 12:10

alexandrnikitin mentioned this pull request Nov 15, 2016

Extending auto value #234

Closed

zvirja force-pushed the add-extensibility-point-for-custom-auto-providers branch from f91205b to db8c2e0 Compare November 15, 2016 20:43

alexandrnikitin merged commit 54a4823 into nsubstitute:master Nov 16, 2016

alexandrnikitin mentioned this pull request Nov 16, 2016

Spike for #150 (Allow direct use of SubstituteFactory) #156

Closed

dtchepak added a commit that referenced this pull request Nov 17, 2016

Update changelog for custom auto-value provders (#259)

55e8d8b

Acknowledge work done in #234, #259, and #156.

zvirja deleted the add-extensibility-point-for-custom-auto-providers branch November 17, 2016 10:44

zvirja mentioned this pull request Dec 22, 2016

Rework nsubstitute integration approach AutoFixture/AutoFixture#726

Closed

zvirja mentioned this pull request Mar 29, 2017

New governance model AutoFixture/AutoFixture#703

Closed

alexandrnikitin mentioned this pull request May 16, 2017

out arguments not working in 2.0.3 #305

Closed

zvirja mentioned this pull request Sep 11, 2017

Rework NSubstitute integration approach AutoFixture/AutoFixture#832

Merged

alexandrnikitin mentioned this pull request Oct 25, 2017

Calling stubbed methods with out params only works once #334

Closed

kekchpek mentioned this pull request Oct 28, 2023

Customize SubstituteFactory. #751

Closed

Add extensibility point for custom auto providers #259

Add extensibility point for custom auto providers #259

Conversation

zvirja commented Nov 8, 2016 • edited Loading

alexandrnikitin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zvirja Nov 11, 2016 • edited Loading

Choose a reason for hiding this comment

zvirja Nov 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zvirja Nov 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zvirja Nov 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zvirja Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zvirja Nov 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zvirja Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zvirja Nov 14, 2016 • edited Loading

Choose a reason for hiding this comment

zvirja commented Nov 8, 2016 • edited Loading

zvirja commented Nov 11, 2016

alexandrnikitin commented Nov 11, 2016

zvirja commented Nov 11, 2016 • edited Loading

dtchepak commented Nov 12, 2016

alexandrnikitin commented Nov 15, 2016 • edited Loading

zvirja commented Nov 15, 2016

dtchepak commented Nov 17, 2016

zvirja commented Nov 17, 2016

dtchepak commented Nov 17, 2016

alexandrnikitin commented Nov 17, 2016

zvirja commented Nov 22, 2016 • edited Loading

alexandrnikitin commented Nov 22, 2016

zvirja commented Nov 22, 2016 • edited Loading

zvirja commented Nov 8, 2016 •

edited

Loading

zvirja Nov 11, 2016 •

edited

Loading

zvirja Nov 14, 2016 •

edited

Loading

zvirja Nov 15, 2016 •

edited

Loading

zvirja Nov 15, 2016 •

edited

Loading

zvirja Nov 8, 2016 •

edited

Loading

zvirja Nov 14, 2016 •

edited

Loading

zvirja Nov 8, 2016 •

edited

Loading

zvirja Nov 14, 2016 •

edited

Loading

zvirja commented Nov 8, 2016 •

edited

Loading

zvirja commented Nov 11, 2016 •

edited

Loading

alexandrnikitin commented Nov 15, 2016 •

edited

Loading

zvirja commented Nov 22, 2016 •

edited

Loading

zvirja commented Nov 22, 2016 •

edited

Loading