How can I mock a function with ref parameter? about moq4 HOT 32 CLOSED

@stakx

1. If anything, I think Callback is in the right, and if you don't support ref then fail in Returns as well. If I'm a user, I might do something like mock.Setup(mc => mc.Bar(ref myBool)).Returns( (bool b) => { b = true; return 42;} ); and then get confused when I call b = false; mock.Object.Bar(ref b) and b is still false.
2. I'm all for the new overload. As for Castle DynamicProxy support, I believe FaekItEasy are using it and they do have some form of ref/out support via the AssignsOutAndRefParameters method. Can't say I'm crazy about this API as it requires the user to remember applying it, and passing in an object[] isn't too hot either (not even sure they validate the signature in terms of ref/out count and type).
3. I'm not sure how It.IsXXX expressions would work with ref/out. Obviously you can do neither mock.Setup(m => m.Foo(ref It.IsAny<bool>())) nor var any = It.IsAny<bool>(); mock.Setup(m => m.Foo(ref any)); (the latter being equivalent to any = false).

from moq4.

I agree for the sake of backwards compatibility (as an MS employee I know a thing or two about that 😅). That being said, I hope you'll consider consolidating the ref behavior for Moq 4.9 (or possibly 5) , ideally to be more strict and match Callback...

from moq4.

@ohadschn - Yes, I would really like Callback and Returns to act in the same way... eventually.

I'm not exactly a fan of the following suggestion, but perhaps we could make Returns more strict in (let's say) in 4.10, then offer some kind of global compatibility switch in Moq that lets users opt out to get the old, more lenient behaviour back. Not thrilled by global switches, but perhaps--given how long Moq has acted as it presently does--we really need to leave the decision up to the end user whether they want convenience or strictness.

I'll get working on these changes in the next few days (excluding the compat switch).

from moq4.

I think the merit for the switch is more on the grounds of backwards compatibility than user convenience (they don't really know what's good for them, just look at all the JS developers 😉).

from moq4.

have a class constraint and verify it's a delegate in reflection

I've tried this, too. It didn't work, there were still occurrences in the test suite where the C# compiler would still pick the wrong overload. Definitely safer to either give the new method a different name, or give up on the generic parameter altogether.

Since this is essentially a new method, how about keeping the generics and simply changing the method name (e.g. CallbackDelegate/ReturnsDelegate)?

Thanks for pointing out this possibility, I never even thought of this. :) I would however much prefer, and advise, that we stay with Callback and Delegate, for these reasons:

• The new methods fundamentally do the same thing as the existing ones: You're handing them a piece of code to be executed at certain times during an invocation; the delegate type used is merely a technical detail that depends (for the most part) on the signature of method being set up.

• I have the impression that there's been a trend in Moq's API to merge similar methods into one method group. (For example, Setup can do what SetupGet does; then there's an ongoing effort to merge ReturnsAsync, ThrowsAsync, etc. with their non-Async counterparts.)

• (Minor point:) The resulting syntax would be even less terse with different method names.

So I think we should continue with Callback and Returns overloads. Is this OK with you? (People will still have the option of defining their own generic extension method, and if those break proper method resolution at compile time, then they have the means to fix it themselves.)

from moq4.

@ohadschn - regarding C# missing ability to deal with ref returns in expression tree lambdas:

Maybe you could open a bug in .NET core or something?

Done. See dotnet/csharplang#158 (comment).

from moq4.

Well you could argue that this is a slightly different use case that merits maybe something like CallbackRef or ReturnsOutRef, but your arguments are good too. Doing Callback(new ExecuteCallback(...)) isn't the end of the world, and really isn't that much different from Callback<ExecuteCallback>(...).

Since type safety isn't static either way (you have to check the Setup expression using reflection), I view it mainly as a matter of taste.

Nice find on the csharplang issue BTW!

from moq4.

@ohadschn - All done. Will start on your issue about Returns lambda signature validation in a bit.

@danicomas, @mattthr - It's been a while, but FYI the upcoming Moq version 4.8 will allow you do set up methods with ref or out parameters, and even assign to by-ref parameters as you'd expect.

public class Hola
{
    // that's what you want to set up a callback for:
    public virtual void DoSomething(ref string a) { }
}

// in order to do so, you will need a delegate with a matching signature:
delegate void DoSomethingAction(ref string a);

[Fact]
public void Test()
{
    string o = "1";
    var mock = new Mock<Hola>();            

    mock.Setup(x => x.DoSomething(ref o))
        .Callback(new DoSomethingAction((ref string a) => a = "2"));

    mock.Object.DoSomething(ref o);

    Assert.Equal("2", o)
}

The syntax is admittedly not super nice; the discussion above shows how we got here.

Hope this helps!

from moq4.

Questions should be directed to [email protected]

from moq4.

Yes @kzu , but is possible or not? If is not possible it is an issue.

from moq4.

You can mock, yes. You can't capture the value via a callback though.

from moq4.

@kzu but how? It throws an exception.

from moq4.

Hi,

Thread necromancy. Is there any chance of progress on this? I love Moq and I've used it exclusively on many projects spanning several years but I've just bumped up against this issue and it's quite frustrating. There's now a chunk of my project I can't cover, and I can't just pull in a new mocking framework :(

Cheers,
Matt

from moq4.

I'd gladly accept a PR if this is critically important to you.

Thanks!

On Mon, Jun 20, 2016, 12:49 PM mattthr [email protected] wrote:

Hi,

Thread necromancy. Is there any chance of progress on this? I love Moq and
I've used it exclusively on many projects spanning several years but I've
just bumped up against this issue and it's quite frustrating. There's now a
chunk of my project I can't cover, and I can't just pull in a new mocking
framework :(

Cheers,
Matt

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#105 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAKW63ER6xyh25mAU2tuwN_yK180Lnusks5qNraAgaJpZM4BxDc-
.

from moq4.

@kzu PR ... is that a pull request? Forgive my ignorance, I'm not a regular user of GitHub. Since I'm not writing a fork I'm not sure how best to do that, so I'll frame my problem below.

Given a setup such as this:

public class FileParser : IFileParser
{
    public void ReadFirstTwoLines(ref string[] first, ref string[] second)
    {
        using(StreamReader reader = new StreamReader("myfile.txt"))
        {
            first = reader.ReadLine().Split(',');
            second = reader.ReadLine().Split(',');
        }
    }
}

public class Demo
{
    private IFileParser _fileParser;

    public Demo(IFileParser fileParser)
    {
        _fileParser = fileParser;
    }

    public int CountFields()
    {
        string[] firstRow = null;
        string[] secondRow = null;

        ReadFirstTwoLines(ref string[] firstRow, ref string[] secondRow);
        return (firstRow.Length + secondRow.Length);
    }
}

I want to be able to write a unit test that can check CountFields is working correctly. As it stands, if I do this:

[TestMethod]
public Assert_Something()
{
    string[] firstline = new string[2] { "1", "2" };
    string[] secondline = new string[2] { "1", "2" };

    Mock<IFileHelper> mf = new Mock<IFileHelper>;
    mf.Setup(m => m.ReadFirstTwoLines(ref firstline, ref secondline)).Verifiable();

    Demo d = new Demo(mf.Object);
    Assert.AreEqual(4, d.CountFields();
}

The method will error because the ref values will come out as null. If I change the mock setup to

    mockFileParse.Setup(m => m.ReadFirstTwoLines(ref firstline, ref secondline))
                    .Callback<string[], string[]>((fl, sl) =>
                    {
                        fl = firstline;
                        sl = secondline;
                    });

I get the error "Invalid callback. Setup on method with parameters (String[]&,String[]&) cannot invoke callback with parameters (String[],String[])".

I don't believe there's a way round this scenario using Moq as it stands, although I'd love to be proved wrong.

from moq4.

Yup, seems like a bug. I wonder why they have to be ref if they are purely
output parameters.

I hardly use ref parameters myself, which is why I suggested a PR if this
is a blocking issue for you.

Thanks

On Tue, Jun 21, 2016, 6:32 AM mattthr [email protected] wrote:

@kzu https://github.com/kzu PR ... is that a pull request? Forgive my
ignorance, I'm not a regular user of GitHub. Since I'm not writing a fork
I'm not sure how best to do that, so I'll frame my problem below.

Given a setup such as this:

public class FileParser : IFileParser
{
public void ReadFirstTwoLines(ref string[] first, ref string[] second)
{
using(StreamReader reader = new StreamReader("myfile.txt"))
{
first = reader.ReadLine().Split(',');
second = reader.ReadLine().Split(',');
}
}
}

public class Demo
{
private IFileParser _fileParser;

public Demo(IFileParser fileParser)
{
    _fileParser = fileParser;
}

public int CountFields()
{
    string[] firstRow = null;
    string[] secondRow = null;

    ReadFirstThreeLines(ref string[] firstRow, ref string[] secondRow);

    return (firstRow.Length + secondRow.Length);
}

}

I want to be able to write a unit test that can check CountFields is
working correctly. As it stands, if I do this:

[TestMethod]
public Assert_Something()
{
string[] firstline = new string[2] { "1", "2" };
string[] secondline = new string[2] { "1", "2" };

Mock<IFileHelper> mf = new Mock<IFileHelper>;
mf.Setup(m => m.ReadFirstTwoLines(ref firstline, ref secondline)).Verifiable();

Demo d = new Demo(mf.Object);
Assert.AreEqual(4, d.CountFields();

}

The method will error because the ref values will come out as null. If I
change the mock setup to

mockFileParse.Setup(m => m.ReadFirstTwoLines(ref firstline, ref secondline))
                .Callback<string[], string[]>((fl, sl) =>
                {
                    fl = firstline;
                    sl = secondline;
                });

I get the error "Invalid callback. Setup on method with parameters
(String[]&,String[]&) cannot invoke callback with parameters
(String[],String[])".

I don't believe there's a way round this scenario using Moq as it stands,
although I'd love to be proved wrong.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#105 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAKW69Lf5BHKfPQLQ0SC5eRyryPAVAnbks5qN6-tgaJpZM4BxDc-
.

from moq4.

@kzu The real code is more complex, but in truth they probably don't need to be ref. I can certainly try to refactor using out or even just passing a custom DTO to hold the data instead. As far as I'm aware, out also causes similar issues in Moq?

Just seems like a curious oversight and obviously I didn't know this would be an issue when I wrote the code (I write test after rather than TDD, don't shoot me!).

from moq4.

Hello everyone, I use a extensive's method, it is called IgnoreRefMatching. I found that class at Stackoverflow, but I lost the reference. Here is the class, I only use with ref parameter:

`
using Moq;
using Moq.Language;
using Moq.Language.Flow;
using System;
using System.Collections;
using System.Collections.Generic;
using System.Linq.Expressions;
using System.Reflection;

namespace Test.Common.Moq
{
public static class MoqExtension
{
public delegate void RefAction<TParam1, TParam2, TParam3, TParam4, TRef>(TParam1 param1, TParam2 param2, TParam3 param3, TParam4 param4, ref TRef refVal1);

    public static IReturnsResult<TMock> RefCallback<TParam1, TParam2, TParam3, TParam4, TRef, TMock>(
       this ICallback mock,
       RefAction<TParam1, TParam2, TParam3, TParam4, TRef> action) where TMock : class
    {
        //ToDo: Código portado da versão 4.5 do .Net Framework para versão Core 1, porém ainda não testado.
        mock.GetType().GetTypeInfo().Assembly.GetType("Moq.MethodCall").GetMethod("SetCallbackWithArguments",
               BindingFlags.InvokeMethod | BindingFlags.NonPublic | BindingFlags.Instance).Invoke(mock, new object[] { action });

        return mock as IReturnsResult<TMock>;

        //Código da versão 4.5 do .Net Framework
        //mock.GetType().Assembly
        //       .GetType("Moq.MethodCall")
        //       .InvokeMember("SetCallbackWithArguments",
        //       BindingFlags.InvokeMethod | BindingFlags.NonPublic | BindingFlags.Instance,
        //       null,
        //       mock,
        //       new object[] { action });

        //return mock as IReturnsResult<TMock>;
    }

    public static ICallback IgnoreRefMatching(this ICallback mock)
    {
        try
        {
            FieldInfo matcherField = typeof(Mock).GetTypeInfo().Assembly.GetType("Moq.MethodCall").GetField("argumentMatchers", BindingFlags.NonPublic | BindingFlags.GetField | BindingFlags.SetField | BindingFlags.Instance);

            IList argumentMatchers = (IList)matcherField.GetValue(mock);
            Type refMatcherType = typeof(Mock).GetTypeInfo().Assembly.GetType("Moq.Matchers.RefMatcher");
            FieldInfo equalField = refMatcherType.GetField("equals", BindingFlags.NonPublic | BindingFlags.GetField | BindingFlags.SetField | BindingFlags.Instance);

            foreach (object matcher in argumentMatchers)
            {
                if (matcher.GetType() == refMatcherType)
                    equalField.SetValue(matcher, new Func<object, bool>(delegate (object o) { return true; }));
            }

            return mock;
        }
        catch (NullReferenceException)
        {
            return mock;
        }
    }

    public static IThrowsResult IgnoreRefMatching(this IThrowsResult mock)
    {
        try
        {
            FieldInfo matcherField = typeof(Mock).GetTypeInfo().Assembly.GetType("Moq.MethodCall").GetField("argumentMatchers", BindingFlags.NonPublic | BindingFlags.GetField | BindingFlags.SetField | BindingFlags.Instance);

            IList argumentMatchers = (IList)matcherField.GetValue(mock);
            Type refMatcherType = typeof(Mock).GetTypeInfo().Assembly.GetType("Moq.Matchers.RefMatcher");
            FieldInfo equalField = refMatcherType.GetField("equals", BindingFlags.NonPublic | BindingFlags.GetField | BindingFlags.SetField | BindingFlags.Instance);

            foreach (object matcher in argumentMatchers)
            {
                if (matcher.GetType() == refMatcherType)
                    equalField.SetValue(matcher, new Func<object, bool>(delegate (object o) { return true; }));
            }

            return mock;
        }
        catch (NullReferenceException)
        {
            return mock;
        }
    }

}

}
`

And part of my test is:

`
int total = 0;

_mockRepository.Setup(x => x.List(It.IsAny<Int32?>(), It.IsAny(), It.IsAny(), It.IsAny<Boolean?>(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), ref total)).Returns(lst).Callback<Int32?, Int32, Int32, Boolean?, Int32, Int32, String, String, Int32>((CallParamIdCliente, CallParamIdUsuario, CallParamIdRegra, CallParamAssociado, CallParamStartRowIndex, CallParamPageSize, CallParamFiltro, CallParamOrderBy, CallParamTotal) => ValidCallBackTipoAssociacao(CallParamFiltro)).IgnoreRefMatching();
`

P.s.: I use .Net Core 1.1

from moq4.

Thanks. For those who (will) struggle like me, you can use @jeremybeavon's gist as well; RefCallback was not working for me. However, you can combine Jeremy's callback and the IgnoreRefMatching function from above.

https://gist.github.com/jeremybeavon/36334a937d6c405831b9

from moq4.

While looking into #445, I've stumbled upon an inconsistency in Moq that could help us resolve this issue here.

We know that .Callback expects its parameters to match (in ref-ness) with the setup method's parameters. Interestingly though, .Returns does not have that restriction! It also turns out that the ref-check in .Callback could be disabled without any harmful consequences. Disabling this check would immediately make it possible to set up a callback for methods with ref parameters. (But it wouldn't offer any way of changing the passed in arguments since none of them are passed by-ref... this is a consequence of there only being Callback overloads for Actions and Funcs... and possibly of Castle DynamicProxy not having that kind of support for by-ref arguments (need to check this)).

Suggested changes:

1. Let's make .Callback less strict by having it ignore ref-ness of parameters when given an Action or Func callback. This gives people a convenient way of specifying callbacks for methods that have by-ref parameters.

2. Add a new overload Callback<TDelegate>(TDelegate callback) that allows people to pass a callback of any delegate type, even one with by-ref parameters. Unlike the existing overloads, this one would additionally match parameters based on their ref-ness (like present-day Callback still does). This gives people a way of actually modifying by-ref parameters inside the callback. (Need to fact-check this, Castle DynamicProxy possibly doesn't offer that kind of support for by-ref arguments.)

Any opinions on this?

from moq4.

@ohadschn, thanks for the feedback.

1. If anything, I think Callback is in the right, and if you don't support ref then fail in Returns as well.

In principle, I totally agree with you. It would be logically correct if the Returns callback had to have a matching signature, and to throw an exception otherwise. Making this change, however, means making Moq stricter, and I'm having two concerns here:

• It will quite likely break existing user code. (I'm not willing to do that on the short term, we should at least announce this first, then make the change in a later version.)

• It will diminish convenience. I imagine there are a lot of situations where you're mocking a method having ref parameters, but you don't really care about them being ref; you just want to read their values to derive a return value. This use case would be broken by what you're suggesting. You'd be required to first define a delegate type whose signature matches that of the mocked method, just so you can use Returns. The whole point of using Moq, however, is freeing you of manually creating types (implementations) at design-time.

The last point (convenience) is why I think that not matching ref-ness (but giving users the option via the new overload) would actually be a feature, not a bug.

I agree that this "feature" can lead to confusion. One way out of this might be to create some Roslyn analyzers that would give informational hints or warnings if you try to use the API incorrectly, such as when you're trying to assign to a by-value callback parameter... but I'm daydreaming now. :)

2. I'm all for the new overload. As for Castle DynamicProxy support, I believe [FakeItEasy] are using it and they do have some form of ref/out support via the AssignsOutAndRefParameters method. Can't say I'm crazy about this API as it requires the user to remember applying it, and passing in an object[] isn't too hot either (not even sure they validate the signature in terms of ref/out count and type).

I think we can do better than requiring users to pass around object[] args arrays. I've run a few quick experiments, adding the overload should be possible and even fairly trivial. I'll prototype something in the next few days.

3. I'm not sure how It.IsXXX expressions would work with ref/out.

That's a tough one. There's two aspects here:

• Semantics: What does ref It.IsAny<T>() even mean? I suppose it helps to think of ref as a combination of a regular "in" parameter, and of an out parameter. The latter don't need to be matched. So I think ref It.IsAny<T>() should mean what It.IsAny<T>() means: match against the value passed in (regardless of the fact that it has been passed by reference instead of by value).

• Implementation: Given C# 7's new "ref returns" and "ref locals" feature, creating an It.IsAnyByRef<T>() method that returns a ref T has become feasible. (We can't overload It.IsAny<T>() based on return type alone, thus a different method name.) The showstopper, however, is that the compiler won't allow "ref returns" in a LINQ expression tree, which is what .Setup takes as input. (Apart from getting the compiler changed, the only 100 % solution I can see here is to change Setup to accept an Action or Func, then decompile that into an expression tree at runtime. But that's quite a bit of work.)

from moq4.

@stakx

1. Totally agree about breaking existing code, no doubt this should be in a major release or something. I disagree about convenience though, IMHO erring on the side of caution is better.
2. Agree about the semantics, bummer about the expression tree limitation :( Maybe you could open a bug in .NET core or something?

from moq4.

Maybe you could open a bug in .NET core or something?

It seems the expression tree API hasn't received a lot of love in the past. Nevertheless, that's good idea. 👍

Totally agree about breaking existing code, no doubt this should be in a major release or something. I disagree about convenience though, IMHO erring on the side of caution is better.

OK, then I suggest the following for Moq 4.8:

Callback:

• We add a new overload Callback<TActionDelegate>(TActionDelegate callback).
• We leave the existing Callback overloads as they are for now (i.e. parameters must match in ref-ness), either (pick the reasoning you like better) because their current behaviour is logically correct, or because we're not in agreement yet. :-)

Returns:

• We add a new overload Returns<TFuncDelegate>(TFuncDelegate callback).
• We leave the existing Returns overloads as they are for now (i.e. parameters need not match in ref-ness), because we don't want to cause an unannounced breaking change.

(We possibly need a new overload for Raises as well, I'll investigate that.)

TL;DR: We keep the existing Callback and Return overloads as they are, but add new overloads that accept a custom delegate type.

Do you agree that these would be good changes for now?

from moq4.

@ohadschn - the new method overloads are pretty much ready (see PR above). There's one unfortunate thing. Ideally, the new overloads would've looked like this:

• Callback<TDelegate>(TDelegate callback) where TDelegate : Delegate
• Returns<TDelegate>(TDelegate valueFunction) where TDelegate : Delegate

This would've allowed us to do this:

delegate void ExecuteCallback(ref Command _);

Command _;
mock.Setup(m => m.Execute(ref _)).Callback<ExecuteCallback>((ref Command command) => …);
                                          ^^^^^^^^^^^^^^^^^

Unfortunately, the C# compiler doesn't allow that type constraint. Unless we start playing around with Fody or another IL weaver, we need to drop it. Without the constraint, however, the methods compete with e.g. Callback(TReturn value) and the compiler ends up picking the wrong overload in some fairly frequent cases. So generics have to be given up, too:

• Callback(Delegate callback)
• Returns(Delegate valueFunction)

mock.Setup(m => m.Execute(ref _)).Callback(new ExecuteCallback((ref Command command) => …));
                                           ^^^^^^^^^^^^^^^^^^^^                          ^

Not quite as terse, but still working.

from moq4.

Since this is essentially a new method, how about keeping the generics and simply changing the method name (e.g. CallbackDelegate/ReturnsDelegate)?

As for the constraint, IMO this would be good enough: https://stackoverflow.com/a/191949/67824 (basically have a class constraint and verify it's a delegate in reflection, no big deal here since reflection is used for validation anyway).

from moq4.

Good stuff @stakx !

I take it the sample works because o is still "1" at the call site right? If you added o = "foo" right before mock.Object.DoSomething(ref o); it wouldn't have worked right?

Did you end up implementing It.IsAnyByRef<T>() ?

from moq4.

If you added o = "foo" right before mock.Object.DoSomething(ref o); it wouldn't have worked right?

That's correct. Think of it like this:

var trigger = "0"; // holds the value that the setup will try to match against
mock.Setup(m => m.Method(ref trigger))
    .Callback(new MethodHandler((ref string x) => x = "1"));

var value = trigger; // use trigger as input so setup is matched
Assert.Equal("0", value); // before
mock.Object.Method(ref value); // value gets aliased as parameter x in callback
Assert.Equal("1", value); // after

Did you end up implementing It.IsAnyByRef<T>() ?

No, for the simple reason that I don't see a way to do it, given the current language / compiler limitations. The best I could think of is something like:

mock.Setup(m => m.MethodWithByRefParams(...))
    .DoNotMatchByRefParams()
    ...;

I guess it'd be best to sketch out, discuss, and track such feature (whatever its concrete form) in a separate, new issue.

from moq4.

Oh right, I forgot about the expression tree issue. While DoNotMatchByRefParams is not ideal, it certainly is better IMO than not being able to achieve this at all...

Do you want me to open an issue or would you prefer to open it?

from moq4.

@ohadschn: Go right ahead. :-)

from moq4.

When will version 4.8 be released? :)

from moq4.

No fixed date yet; my guess is that it'll take another few weeks. I'd like to be extra careful with the planned features, as they could easily cause breaking changes.

from moq4.

@asyle83 - A pre-release of Moq 4.8.0 (4.8.0-rc1) is now available on NuGet.

from moq4.