Provide a mechanism for capturing arguments passed to Fakes

[blairconrad]

Provide a mechanism for capturing arguments passed to Fakes

[blairconrad]

Thanks for the feedback, @PiotrKlecha. @thomaslevesque and I settled on the interface we presented at the top of this issue after careful deliberation and we think it's the best option available given the constraints of the framework (and .NET). We hope it'll work for most of our client. That being said, we appreciate your input and are happy to talk about it. Let me start by explaining a little more about why we chose what we did, in the context of your comment

it violates the traditional Arrange/Act/Assert flow of a test by cluttering the Arrange part with code that has nothing to do with an actual test set up.

I see your point. While we're setting up behaviour, it only exists to support the Assert. Sort of like a weaker version of an expectation feeding into a "verify" step as some frameworks use.

I think that method call verification should entirely happen in the Assert part of a test where it belongs and capturing arguments is not really needed to acheive this - FakeItEasy already offers a way to do exactly that [Fake.GetCalls(fake)]

It's true. And it's a pretty good way to do this sort of thing. I like your extension method. Maybe we should consider exposing something like that and promoting it. We had discussed something very much like this.

The approach has the slight downside of having to call GetArgument[int] (or GetArgument[string]) and correlate indexes with arguments. This was a point against going down this route.

The stronger point was that it fails if the arguments are reused and are mutable. Examining the calls received after the fact loses the state of the arguments as they were provided. Only some action taken at the time the call is received can preserve the history. Please take a gander at #1967, and specifically the new docs I wrote for the proposed argument-capturing feature. You can see the problem and how arranging to preserve state can overcome it.

We know that people do require this sort of behaviour from time to time and we want to enable it.

Adding something like GetLastCall and other helpful methods could conceivably live alongside the solution in my PR, but then we'd have 2 systems, which means more burden on people to learn (and us to document, test, maintain, etc.).

Anyhow, take a gander if you would. I'd like to hear what you think.

[PiotrKlecha]

all required methods are pretty hard to discover.

Yes. A downside of us working primarily with fakes that are of the same type as the faked thing, rather than Fake<T> all the time. Were I writing a library from scratch, I might take the other approach.

IMHO the current approach is superior in at least 99% of use cases, it helps keeping tests much cleaner and more readable. No solution is perfect but current drawbacks seem to be really minor ones, especially considering all the benefits.

I'd started noodling with something like this on my own before seeing your comment. As I sit right now, I think it's my favourite candidate for freezing arguments. And I have less problem with setting this up in the arrange portion of the test than you do 😉.

I guess I'm a little bit paranoid about it and I might be overreacting 😉

To me, this seems worse than putting anything explicitly in the arrange portion. Hiding something away in the implicit options makes tests less readable, in my opinion, and is still an action occurring in the arrange (or at least before act) phase.

I agree that implicit options might be confusing, it wasn't probably a good idea in this case, but I mentioned them primarily to discuss some other possibilities.

I notice some downsides of storing the captured arguments in separate CapturedArgument objects:

• multiple additional objects are required if more arguments need to be captured

true, but I don't think it's a huge deal. It's a tradeoff. If people mostly access one argument, it's not a problem at all, and dealing with a list of argument-typed values would be more convenient

• arguments from the same call are separated from each other - they will probably be stored with the same index on the Values list, but will still need to be manually matched, if some dependencies between them have to be verified,

100% true, but if you only care about the one argument, it's easier to not have to wade through the ArgumentCollection

• one CapturedArgument object could be used to store arguments of same type from different methods

It could. Or even arguments of the same type from the same method (if the method had multiple parameters of the same type). Mistakes happen, but I'm not sure the likelihood and bad outcomes are dire enough to make this a huge concern.

Even so, I'm not as sold on the external CapturedArgument instances as I was. I continue to play with getting data from the captured calls.

I admit that capturing the state of more than one object is not a very common scenario, but I thought those cases are still worth mentioning for the sake of the discussion.

[thomaslevesque]

This change is finally merged! Special thanks to @Miista, whose FakeItEasy.Capture project gave us the idea that finally unblocked this feature. We didn't follow exactly the same approach (e.g. we don't use implicit conversion), but you'll probably notice the similarity.

The docs for the new feature are (temporarily, until we release this change) visible here.

[blairconrad]

@PiotrKlecha, this is indeed what I thought @Miista was asking about earlier. Your idea does make quite a bit of sense. However it seems that most users of mocking libraries have come to expect to create capturing objects and read the results from them. Forcing anyone who wanted to do so to provide a possibly do-nothing function isn't very friendly, IMO. And then saying to them "if you don't want to provide this method, you should instead interrogate the recorded calls" feels to me unintuitive.

[PiotrKlecha]

Indeed. We'd be interested in hearing of such an interface. Have you one to propsse?

Not yet, but I'll try to come up with something reasonable - I guess it would be better to create a new issue for this topic.

[PiotrKlecha]

Hello, I have some suggestions on this topic - I have been using the Invokes method to capture and verify arguments for a while and now I think that using or extending this approach is not the right way to go. The problem is that it violates the traditional Arrange/Act/Assert flow of a test by cluttering the Arrange part with code that has nothing to do with an actual test set up. I think that method call verification should entirely happen in the Assert part of a test where it belongs and capturing arguments is not really needed to acheive this - FakeItEasy already offers a way to do exactly that:

var fake = A.Fake<IService>();

// exercise SUT

var argument = Fake.GetCalls(fake)
	.Matching<IService>(s => s.SomeMethod(A<string>._))
	.Select(call => call.GetArgument<string>(0))
	.LastOrDefault();

// verify argument

The above syntax could be improved a little to make it more readable and adding some extensions helps a lot:

var fake = A.Fake<IService>();

// exercise SUT

var argument = fake
	.GetLastCall(s => s.SomeMethod(A<string>._))
	.GetArguments((string a) => a);

// verify argument

I use custom extensions in my project, but having them available out of the box would be great.

IMHO the mechanism proposed in this issue will only encourage users to write tests in an inappropriate way and it would be much more beneficial to simplify the existing syntax for extracting arguments from called methods e.g. using extensions mentioned above.

[PiotrKlecha]

I think that method call verification should entirely happen in the Assert part of a test where it belongs and capturing arguments is not really needed to acheive this - FakeItEasy already offers a way to do exactly that [Fake.GetCalls(fake)]

It's true. And it's a pretty good way to do this sort of thing. I like your extension method. Maybe we should consider exposing something like that and promoting it. We had discussed something very much like this.

I actually didn't think that Fake.GetCalls could be of much use when I originally read about it in the docs, but I accidentally ran into the Matching extension while working with FakeItEasy source code. Only then I realized how it could help making call argument verification much more convenient. This approach could definately use some promotion, because all required methods are pretty hard to discover.

The approach has the slight downside of having to call GetArgument[int] (or GetArgument[string]) and correlate indexes with arguments. This was a point against going down this route.

This is quite similar to other methods that process FakeObjectCalls e.g. Invokes or ReturnsLazily - additional overloads/extensions seem to be a reasonable solution in this case as well.

The stronger point was that it fails if the arguments are reused and are mutable. Examining the calls received after the fact loses the state of the arguments as they were provided. Only some action taken at the time the call is received can preserve the history. Please take a gander at #1967, and specifically the new docs I wrote for the proposed argument-capturing feature. You can see the problem and how arranging to preserve state can overcome it.

We know that people do require this sort of behaviour from time to time and we want to enable it.

This is probably the trickiest problem to solve using my approach and I do not have a fully satisfying idea, but perhaps we could discuss it anyway and maybe we come up with something better. I agree that only an action taken at the time of the call will be able to preserve argument state, so this implies that this action has to be defined before the call happens. One way to acheive this could be adding a new dedicated capture callback:

var fake = A.Fake<IService>();

A.CallTo(() => fake.SomeMethod(A<IList<string>>._))
	.CapturesUsing((IList<string> list) => string.Join(", ", list));

This would be a little bit different from the regular call set up, however, is still against my original preference of keeping the test's Arrange part clean. Another way to capture the arguments could be using a more generic method, e.g. with a IInterceptionListener, which could then be attached at the moment of a Fake creation (or even with implicit creation options), so it could be moved away from the test, so the Arrange part remains cleaner. However, I'm not sure if such a generic capture method itself would then be readable enough.

Argument values processed with the attached callback would be stored in the FakeObjectCall (a new property e.g. CapturedValues) and then extracted, possibly using another extension:

var argument = fake
	.GetLastCall(s => s.SomeMethod(A<IList<string>>._))
	.GetCapturedValues((string a) => a);

This unfortunately has a drawback of not being type safe, since the stored captured values probably could not be strongly typed and would require the user to provide callbacks of correct types. Perhaps this is not a serious problem after all, since currently available Arguments in FakeObjectCall have the same limitation.

Adding something like GetLastCall and other helpful methods could conceivably live alongside the solution in my PR, but then we'd have 2 systems, which means more burden on people to learn (and us to document, test, maintain, etc.).

I guess extensions like GetLastCall (and possibly GetArguments) could be viable on their own, because they simplify operations on FakeObjectCalls and might be used for other purposes than argument verification. I'll gladly share them, if you are interested in adding them to the framework.

Anyhow, take a gander if you would. I'd like to hear what you think.

I notice some downsides of storing the captured arguments in separate CapturedArgument objects:

• multiple additional objects are required if more arguments need to be captured - this might reduce readability of a test,
• arguments from the same call are separated from each other - they will probably be stored with the same index on the Values list, but will still need to be manually matched, if some dependencies between them have to be verified,
• one CapturedArgument object could be used to store arguments of same type from different methods - this might be beneficial in some cases, but on the other hand would likely break the above assumption that arguments from the same call are stored with the same index.

These might be corner cases, but I think I would prefer storing captured arguments in the FakeObjectCall, even if it means sacrificing the type safety provided by CapturedArgument.

[thomaslevesque]

Hi @PiotrKlecha,

Thanks for the feedback!

These might be corner cases, but I think I would prefer storing captured arguments in the FakeObjectCall, even if it means sacrificing the type safety provided by CapturedArgument.

This is an interesting comment. Instead of capturing individual arguments, we could capture the whole calls:

var calls = new List<IFakeObjectCall>();
A.CallTo(...).IsCapturedTo(calls).Returns(...);

And then we can examine the captured calls to see their arguments. It's basically the same thing as using Fake.GetCalls(fake).Matching(...), but avoids repeating the call specification. It also has the same drawbacks, especially regarding mutable arguments...

[blairconrad]

@thomaslevesque, is there much advantage in your latest code? I was thinking we probably wouldn't often want to restrict calls to a method if we're going to examine the captured arguments, but I guess the fake could have multiple methods being exercised.

Still, we make people repeat the A.CallTo to check that calls MustHaveHappened. Is making them repeat to examine the arguments so bad?

I'm examining some syntactic sugar to make it easier to get at the particular sets of arguments now…

[blairconrad]

Oh, bother. I'm not getting notifications and didn't notice your response @PiotrKlecha. I have some thoughts

all required methods are pretty hard to discover.

Yes. A downside of us working primarily with fakes that are of the same type as the faked thing, rather than Fake<T> all the time. Were I writing a library from scratch, I might take the other approach.

Also, these methods don't hang off A like most of the others.

The approach has the slight downside of having to call GetArgument[int] (or GetArgument[string]) and correlate indexes with arguments. This was a point against going down this route.

This is quite similar to other methods that process FakeObjectCalls e.g. Invokes or ReturnsLazily - additional overloads/extensions seem to be a reasonable solution in this case as well.

True. And I've been kind of getting over my objection to this. I think.

A.CallTo(() => fake.SomeMethod(A<IList<string>>._))
  .CapturesUsing((IList<string> list) => string.Join(", ", list));

I'd started noodling with something like this on my own before seeing your comment. As I sit right now, I think it's my favourite candidate for freezing arguments.
And I have less problem with setting this up in the arrange portion of the test than you do 😉.

Another way to capture the arguments could be using a more generic method, e.g. with a IInterceptionListener, which could then be attached at the moment of a Fake creation (or even with implicit creation options), so it could be moved away from the test, so the Arrange part remains cleaner.

To me, this seems worse than putting anything explicitly in the arrange portion. Hiding something away in the implicit options makes tests less readable, in my opinion, and is still an action occurring in the arrange (or at least before act) phase.

I guess extensions like GetLastCall (and possibly GetArguments) could be viable on their own, because they simplify operations on FakeObjectCalls and might be used for other purposes than argument verification. I'll gladly share them, if you are interested in adding them to the framework.

It's true. I'll keep the offer in mind.

I notice some downsides of storing the captured arguments in separate CapturedArgument objects:

• multiple additional objects are required if more arguments need to be captured

true, but I don't think it's a huge deal. It's a tradeoff. If people mostly access one argument,
it's not a problem at all, and dealing with a list of argument-typed values would be more convenient

• arguments from the same call are separated from each other - they will probably be stored with the same index on the Values list, but will still need to be manually matched, if some dependencies between them have to be verified,

100% true, but if you only care about the one argument, it's easier to not have to wade through the ArgumentCollection

• one CapturedArgument object could be used to store arguments of same type from different methods

It could. Or even arguments of the same type from the same method (if the method had multiple parameters of the same type). Mistakes happen, but I'm not sure the likelihood and bad outcomes are dire enough to make this a huge concern.

Even so, I'm not as sold on the external CapturedArgument instances as I was. I continue to play with getting data from the captured calls.

Extension methods on IEnumerable<ArgumentCollection> to "slice" the enumerable into particular arguments (by index or name) may be a decent compromise… They'd basically be fancy versions of Select.

[PiotrKlecha]

I have one more idea that could make defining the capture functions more convenient. I think it would be very useful to declare a capture that would match an argument by its type, e.g.:

var fake = A.Fake<IService>();

A.CallTo(fake)
	.CapturesArgumentsOfType<IList<string>>(list => string.Join(", ", list));

This capture would apply to all matching arguments for any method of the fake:

var list1 = new[] { "1", "2", };
var list2 = new[] { "3", "4", };

fake.SomeMethod(list1); // captures: "1, 2"
fake.OtherMethod(1, "x"); // captures nothing
fake.AnotherMethod(list2, 0, list1, "y"); // captures: "3, 4" and "1, 2"

With this approach the definition of the capture function is simplier, a single capture is sufficient to cover all methods of a Fake and the same function could be attached even to different Fakes (one capture to rule them all 😉 ).

[thomaslevesque]

To be honest, I'm not sure this would be very convenient. If there are multiple parameters of the same type (or if multiple methods have parameters of that type), the captured argument values would all be lumped together in the same list with no way to distinguish them.

[PiotrKlecha]

To be honest, I'm not sure this would be very convenient. If there are multiple parameters of the same type (or if multiple methods have parameters of that type), the captured argument values would all be lumped together in the same list with no way to distinguish them.

Not exactly - assuming that captured arguments are stored in the FakeObjectCall, then one list of captures would contain at most as many items as there are arguments of the called method. But perhaps a common list for all captures is indeed not convenient with this approach - maybe each argument of the call should have its captured value attached? It could then be accessed using a new method e.g. call.GetCapturedValue<T>(0) (similar to existing call.GetArgument<T>(0)).

[blairconrad]

Ugh. I'd had some sidechat with @thomaslevesque about this after these last exchanges, where I expressed my doubts over the original approach and why I was leaning more toward a "mine the recorded calls for captured arguments" approach, but how I wanted some time to settle my thoughts and maybe mock up some approaches, but work and life came upon me and I

1. never got around to the thinking and mocking, and
2. forgot to make a comment here

There's been some great talk over at #1975 about Capturing recently, but I'd like to pull the conversation back here if I could. I will (soon for sure this time) come back with thoughts I've been having. A very abbreviated version of some of my concerns:

1. mutable arguments are a challenge for capturing, but also for our current MustHaveHappened verification. If we're going to "solve" them once, it would be nice to solve them for both cases
2. applying capturing rules that "freeze" incoming arguments, whether they freeze to the same type or to different ones, for some methods but not others (or even some calls to the same method but not others) may end up creating situations that confuse the user. I think "freezing" to a new type exacerbates the potential confusion
3. (something I just thought of while typing this list, which shows that I need more thought) IsSameAs is likely to complicate or be complicated by any approach that's going to "freeze" mutable arguments

I'd been leaning toward the idea of adding convenience methods for extracting captured arguments from recorded calls (not unlike @PiotrKlecha's original approach) and introducing the concept of freezing incoming arguments. I was leery of the confusion that might be introduced by having the "freezer" change the type of the argument for some calls but not others, and was going to propose that the "freezer" be scoped to the Fake and the type of the argument (I'm sure there are downsides to this as well) and be introduced as Fake Creation Option.

I was surprised to learn of FakeItEasy.Capture. I like the approach, but if we were to implement something like it in FakeItEasy, I'd probably have wanted to expand it to cover more corner cases; something more like my original PR for this issue. But since I'd been moving my thoughts more toward mining already recorded calls, I thought "well, it's nice that FakeItEasy.Capture exists for users who like the 'other' approach".

Sadly, that's all I have time for now. Sorry for the confusion.

/cc @Miista, @jrosiek

[blairconrad]

New update. @thomaslevesque and I have been chatting on the side and we convinced me that creating an object to capture arguments (as I did in my original PR and as FakeItEasy.Capture (and most other mocking libraries I've used)) is probably the better path. But then piling on an A<typeparams>.That.IsCapturedTo is a little clunky. I'm altering my original PR to change the A.CallTo syntax slightly and will send it up for review in the next couple of days (famous last words).

[afakebot]

This change has been released as part of FakeItEasy 8.1.0.

[Miista]

@blairconrad: Sorry to comment on a closed issue.

Would it not be more intuitive if captured values were always frozen?

[blairconrad]

Hi, @Miista. Don't feel bad about commenting on a closed issue.

"more intuitive" is a slippery concept. I find different people intuit different things. It would provide a simpler interface with only one entry point. Maybe this is what you mean?

I don't know that this was explicitly discussed during the implementation of this issue, but a reason not to freeze every value is that in the case where there's no benefit to doing so (the argument values are immutable, or just happen never to change), then the API would force users to specify a freezer function that they didn't care about. This would add visual noise. And some additional processing (although an "identity" function would probably be pretty cheap, so maybe we can ignore that part).

[Miista]

My reasoning was that there is an expectation that arguments are captured as-is at the point of invocation. In other words, I (as a user) should not be concerned with whether the argument is mutated after the point of invocation. Does that make sense?

When interrogating the captured values, I would expect the values to be the same as when it was passed in. That is, the value is frozen at the point of capture.

That said, it's not something I've encountered in the four years that FakeItEasy.Capture has been available.

[blairconrad]

I understand now. Thanks for explaining.

It's a not uncommon expectation and has come up a number of times in Stack Overflow (and other) questions over the years. Here's a good example of one such.

Unfortunately, it's much more difficult than people tend to assume. There's no general purpose "save the original value of an argument" function. There's no way to tell how to preserve the state of the input to a Func<T>, for example. This is the main reason that FakeItEasy retains the object passed in and we are (as of this issue) giving users the ability to provide a state-retaining (or freezing) function: only they know what would need to be done to an object to create what's essentially a snapshot.

[PiotrKlecha]

Perhaps it would be more intuitive if the "freezer" function was required for A.Captured. Without this function the A.Captured object is actually redundant, because arguments are already captured by FakeObjectCall's in the same way (by reference/identity). Making the "freezer" mandatory would remove this redundancy and suggest that A.Captured should be used when saving argument state is necessary.

[PiotrKlecha]

Well, I have to admit that I still consider extracting arguments from recorded calls a better approach and now I am among those who have to design test differently depending on whether they need to freeze the argument state or not. I understand that you have to care about the majority of the users in the first place, but maybe FakeItEasy is big enough for another first-class mechanism that would capture the mutable argument state in the calls (if a satisfying interface is proposed, of course).

[blairconrad]

and now I am among those who have to design test differently depending on whether they need to freeze the argument state or not

I sympathize with your disappointment, but this change hasn't made the situation worse. You would've had to test differently regardless.

maybe FakeItEasy is big enough for another first-class mechanism that would capture the mutable argument state in the calls (if a satisfying interface is proposed, of course)

Indeed. We'd be interested in hearing of such an interface. Have you one to propsse?

[blairconrad]

I guess it would be better to create a new issue for this topic.

Definitely!