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

Add new Callback and Returns overloads for setting up methods with ref parameters #468

Merged
merged 7 commits into from
Oct 5, 2017

Conversation

stakx
Copy link
Contributor

@stakx stakx commented Oct 3, 2017

This PR proposes new method overloads for the Callback and Returns setup method that would allow setting up callbacks for methods having ref parameters. They do this by accepting delegates of any delegate type (including such ones having ref parameters); the present overloads only accept Actions or Funcs, respectively.

This would resolve #105.

@stakx stakx force-pushed the ref-callback-return branch 2 times, most recently from c5c12e3 to 31c7269 Compare October 5, 2017 06:48
This completes the method group which currently has only overloads
for `Action` delegates. With the new overload, it is possible to set
up callbacks for methods having by-ref parameters.
This isn't strictly needed just yet since present day `Returns` al-
ready works with set up methods having by-ref parameters, as it does
not perform parameter list type matching as strictly as `Callback`
does.

However it's good to have this overload for API consistency, and it
will be important in a later release, should we decide to make
`Returns` as strict as `Callback` (because then, without this new
method overload, you would no longer be able to set up return call-
backs for methods having by-ref parameters).
Adding the new generic overload `Returns<TDelegate>(TDelegate)` has
an unfortunate side effect. In some cases, the compiler will pick
the new overload over the preexisting `Returns(TResult)`; for example
when `TResult` is `object` and a call `Returns("string")` is made.

Since we cannot constrain generic type parameters to `Delegate`, we
need to make the new overload non-generic to help the compilers in
picking the right overload.

(There's one more edge case, namely when `TResult` is `Delegate`. The
new overload must forward to the preexisting overload in that case.)
@stakx stakx force-pushed the ref-callback-return branch 2 times, most recently from ec16bcd to ebc6bbd Compare October 5, 2017 19:25
Now that the new overload for `Returns` has had to become non-generic,
so should the new `Callback` overload (for consistency).

Also forgot to update the XML documentation comments, bring them in
line again with the actual method signatures.
@stakx stakx merged commit 2cd9690 into devlooped:develop Oct 5, 2017
@stakx stakx deleted the ref-callback-return branch October 5, 2017 19:46
@ajbeaven
Copy link

I think it was due to this change that a previously passing test started failing after upgrading from 4.7.145 to 4.8.0:

public interface IUserRepository
{
  List<TUser> GetUsers<TUser>(UserSearch search) where TUser : User;
  List<TUser> GetUsers<TUser>(UserSearch search, out int totalUsers) where TUser : User;
}

int totalUsers;
unitOfWorkMock.Setup(x => x.Users.GetUsers(It.IsAny<UserSearch>())).Returns(new List<User>());
var result = unitOfWorkMock.Object.Users.GetUsers(new UserSearch(), out totalUsers); // result is null

The fix was to specify the out parameter in the call to Setup.

I'm not sure if this warrants a Breaking change note on the changelog, but it would have saved me a bit of confusion when trying to figure out why my tests started failing.

@stakx
Copy link
Contributor Author

stakx commented Oct 31, 2018

Not sure either, but regardless of whether it's a breaking change or not, it seems to me that your test code used to be written incorrectly (setting up the wrong overload), and adding the second parameter to the setup was the correct fix. Do you agree, or have I misunderstood something?

@ajbeaven
Copy link

Yes you're exactly right and I completely agree. I suppose it probably shouldn't be considered a breaking change if the test was only passing due to a bug, so feel free to ignore this. Thanks for all your contributions to moq, its awesome!

@stakx
Copy link
Contributor Author

stakx commented Oct 31, 2018

@ajbeaven - I'll look at your scenario more closely as soon as I get a spare moment, but unless it turns out to be an unexpected corner case that I overlooked back then, I'll probably leave things as they are now for the reason you mentioned.

Glad to hear that Moq continues to be useful to you!

@devlooped devlooped locked and limited conversation to collaborators Sep 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants