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

Allow the equivalent of It.IsAny with ref parameters in Setup #479

Closed
ohadschn opened this issue Oct 7, 2017 · 15 comments
Closed

Allow the equivalent of It.IsAny with ref parameters in Setup #479

ohadschn opened this issue Oct 7, 2017 · 15 comments
Assignees
Milestone

Comments

@ohadschn
Copy link

ohadschn commented Oct 7, 2017

Following #105, it would be nice to be able to match any value of a passed by ref variable in Setup.

Per dotnet/csharplang#158 that can't currently be achieved using ref returns (e.g. ref T It.IsAnyRef<T>) with the current overloads that take expression trees.

However, it might be doable by accepting a delegate and decompiling it into an expression tree at runtime. That would involve quite some work though, and for something this specific perhaps the following would suffice:

mock.Setup(m => m.MethodWithByRefParams(...))
    .DoNotMatchByRefParams()
    ...;
@stakx stakx added this to the v4.8.0 milestone Nov 29, 2017
@stakx stakx self-assigned this Nov 29, 2017
@stakx
Copy link
Contributor

stakx commented Nov 29, 2017

Like you said, the C# language currently doesn't allow us to do this nicely, and lambda decompilation is possible (see e. g. this proof-of-concept prototype I cooked up a while ago), but will require Moq to become dependent on .NET Standard 2, and it would take some more work—I'd like to keep this open as an option for a 4.9 release or later.

For now, the least we can do is to add a helper method that "disables" matching for ref parameters, like the .DoNotMatchByRefParams() you suggested. I'll look into this.

@stakx
Copy link
Contributor

stakx commented Nov 29, 2017

@ohadschn - Quick question: Do you think a feature switch (Mock.Switches |= Switches.DoNotMatchRefParameters) instead of a setup verb setup.DoNotMatchRefParams() would also be agreeable?

The reason behind this is that I've realised that a .DoNotMatchByRefParams() setup method would only work in combination with mock.Verify() or mock.VerifyAll(), but not when you do a mock.Verify(m => m.Method(...)). We'd have to introduce several new Verify method overloads, which is something I'd really like to avoid.

On the other hand, a feature switch, which is more coarse-grained than the method because it sits at the mock level (instead of at the setup level) could easily be made to apply to all Verify calls:

var mock = new Mock<IHaveMethodsWithRefParameters>();
mock.Switches |= Switch.DoNotMatchRefArguments;
...
mock.Verify(m => m.Method(1, ref dummy, 2), ...);

(I know it's not quite as neat as the method, but it would be a more complete solution. And while we could of course have both, I'd prefer to introduce as few additions to the public API as possible.)

Thoughts?

@ohadschn
Copy link
Author

ohadschn commented Nov 29, 2017

How about something like:

mock.NoRefArgMatching().Verify(...)

Where NoRefArgMatching returns a wrapper for the mock with the switch enabled?
That way you could still decide to match refs on a case by case basis.

@stakx
Copy link
Contributor

stakx commented Nov 29, 2017

That would be a nice solution. Unfortunately, there's the question of how that method call would know when the switch needs to be reset. Also, setting up a temporary context where a "shared" switch is modified means that this method wouldn't be thread-safe / reentrant. 🙁

(Personally, I don't think that Setup nor Verify need to be thread-safe, because having any concurrency or parallelism in the Arrange or Assert stage of a test is perhaps not a good idea anyway. Nevertheless, Moq has in the past seen contributions to make mocks thread-safe wherever possible—perhaps because people also use Moq for purposes other than unit testing—it's probably not my call to reverse that community decision.)

@ohadschn
Copy link
Author

I agree about managing this state, I was actually thinking about creating a new Mock with the same underlying object.. would that be possible?

@stakx
Copy link
Contributor

stakx commented Nov 29, 2017

In theory, perhaps yes. In practice, I'd say this approach isn't worth the required effort.

One would create a new Mock<T> object sharing all internal state with the original except for the switch state. It would take a lot of work to ensure this doesn't cause more trouble than it solves. Just one example, you'd then have several mocks with the exact same name, but comparing them with == or object.ReferenceEquals would reveal that they're not actually the same instance. From a user's perspective, that could be quite confusing. I'd say it's better we don't do this.

@ohadschn
Copy link
Author

ohadschn commented Nov 29, 2017

How about something like

mock.Verify(m => NoRefVerify(m.Method(1, ref dummy, 2)), ...);

Where NoRefVerify simply returns whatever it gets (including void overloads) and is only there for your expression analyzer to be able to spot?

@stakx
Copy link
Contributor

stakx commented Nov 29, 2017

Your last suggestion just gave me an idea. It never occurred to me until now that we could simply do this:

mock.Setup(m => m.Method(, ref It.Ref<int>.IsAny,));
//                          ^^^^^^^^^^^^^^^^^^^^^

The generic parameter obviously isn't in the usual place —it would ideally follow IsAny instead of preceding it—but it should be perfectly feasible:

static partial class It
{
    public static class Ref<T>
    {
        public static T IsAny;
    }
}

(Credits go to @michal-ciechan, the above technique is inspired by what he did in #343.)

@ohadschn
Copy link
Author

Nice! Maybe It.IsAny<int>.Ref?

@stakx
Copy link
Contributor

stakx commented Nov 29, 2017

There's already the It.IsAny<T>() method. The compiler won't allow overloading it with an identically-named type.

@ohadschn
Copy link
Author

Well I suppose you could do ItIs.Any<int>.Ref :)
But really the name isn't that important, whatever works would be great.

@stakx
Copy link
Contributor

stakx commented Nov 29, 2017

All done. As a small bonus, "any" matching for ref parameters is now also directly supported with .Protected() setups:

mock.Protected().Setup("ProtectedMethod",, ItExpr.Ref<int>.IsAny,);

Well I suppose you could do ItIs.Any<int>.Ref :)

Just to answer this, I opted to stay with It.Ref<int>.IsAny so that It remains the single "discovery / entry point" for the standard matchers.

I chose Ref instead of ByRef for two reasons:

  1. ByRef is a keyword in VB.NET, and while this shouldn't cause actual problems, it's often a good idea not to use keywords as identifiers.

  2. ByRef would technically include out parameters. Ref is closer to the C# keyword ref, which is what is really targeted.

@stakx stakx closed this as completed Nov 29, 2017
@stakx
Copy link
Contributor

stakx commented Dec 8, 2017

@ohadschn - I just published a pre-release version 4.8.0-rc1 on NuGet. It has all the new ref stuff (among other things) that we've been discussing in several issues. Feel free to test away.

@asyle83
Copy link

asyle83 commented Feb 9, 2018

Hi, I have a problem with a method that has a ref parameter of type Array:
Method(int param1, string param2, ref Array param3)
param3 is an output parameter and must be not null in order to continue the code execution but I don't know how to mock it.
I tried:
_mock.Setup(m => m.Method(It.IsAny<int>(), It.IsAny<string>(), ref It.Ref<Array>.IsAny));
and also
_mock.Setup(m => m.Method(It.IsAny<int>(), It.IsAny<string>(), ref arrayTest));
where arrayTest is an array populated with some data.

But in any case during the test execution, the ref parameter is always null...
How can I solve?
Thanks

EDIT: Solved


delegate void MethodCallback(int param1, string param2, ref Array param3);

_mock.Setup(m => m.Method(It.IsAny<int>(), It.IsAny<string>(), ref It.Ref<Array>.IsAny)) 
                .Callback(new MethodCallback((int param1, string param2, ref Array param3) => {
                    param3= arrayRes;
                }));

@stakx stakx reopened this Feb 9, 2018
@stakx stakx closed this as completed Feb 10, 2018
@stakx
Copy link
Contributor

stakx commented Feb 10, 2018

Sorry, accidentally pressed button.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants