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

Moq.Protected.Setup() Breaks On Abstract Class with Overloaded Abstract Methods #735

Closed
sdepouw opened this issue Dec 19, 2018 · 13 comments · Fixed by #751
Closed

Moq.Protected.Setup() Breaks On Abstract Class with Overloaded Abstract Methods #735

sdepouw opened this issue Dec 19, 2018 · 13 comments · Fixed by #751

Comments

@sdepouw
Copy link

sdepouw commented Dec 19, 2018

I recently upgraded from Moq 4.5.28 to 4.10.1. After I performed the upgrade (via NuGet) and ran my tests, some failed.

I have the following abstract class:

public abstract class MyAbstractClass
{
    protected abstract void ApplyRule(IDictionary<string, object> tokens);
    protected abstract void ApplyRule(object obj);
    
    // Other stuff below
}

And the following test fixture:

[TestFixture]
public class ApplyShould
{
    private MyAbstractClass _rule;
    private int _timesApplyRuleCalled;

    [SetUp]
    public void SetUp()
    {
        Mock<MyAbstractClass> mockedRule = new Mock<MyAbstractClass>();
        mockedRule.Protected().Setup("ApplyRule", ItExpr.IsAny<IDictionary<string, object>>()).Callback(() => _timesApplyRuleCalled++);
        mockedRule.Protected().Setup("ApplyRule", ItExpr.IsAny<object>()).Callback(() => _timesApplyRuleCalled++);
        _rule = mockedRule.Object;
        _timesApplyRuleCalled = 0;
    }
    
    // Tests below
}

In 4.5.28, this worked fine. But now in 4.10.1, I get the following error:

SetUp : System.InvalidOperationException : Sequence contains more than one matching element
   at System.Linq.Enumerable.SingleOrDefault[TSource](IEnumerable`1 source, Func`2 predicate)
   at Moq.Protected.ProtectedMock`1.Setup(String methodName, Object[] args) in C:\projects\moq4\src\Moq\Protected\ProtectedMock.cs:line 39
   at MyAbstractClassTests.ApplyShould.SetUp() in C:\MyAbstractClassTests\ApplyShould.cs:line 23

I attempted to use the exactParameterMatch argument on the Setup() method to see if that would help differentiate things:

mockedRule.Protected().Setup("ApplyRule", true, ItExpr.IsAny<IDictionary<string, object>>()).Callback(() => _timesApplyRuleCalled++);
mockedRule.Protected().Setup("ApplyRule", true, ItExpr.IsAny<object>()).Callback(() => _timesApplyRuleCalled++);

But that didn't seem to work:

SetUp : System.ArgumentException : Member ObitTokenFixerRuleBase.ApplyRule does not exist.
   at Moq.Protected.ProtectedMock`1.ThrowIfMemberMissing(String memberName, MemberInfo member) in C:\projects\moq4\src\Moq\Protected\ProtectedMock.cs:line 263
   at Moq.Protected.ProtectedMock`1.Setup(String methodName, Object[] args) in C:\projects\moq4\src\Moq\Protected\ProtectedMock.cs:line 41
   at {same stack trace}

My workaround was to rename one of the ApplyRule overloads to differentiate them in code. This makes Moq no longer confused (it doesn't find two methods with the same name). Based on other testing I did, it looks like this is because IDictionary<string, object> can also be object. If I make an abstract class with, say, string and int, Moq has no troubles here.

Is this a bug? Or is this a consequence of changes made to Moq that won't/can't be fixed?

@stakx
Copy link
Contributor

stakx commented Dec 20, 2018

Hi @sdepouw. Thanks for reporting this. I'll take a look at it during the next few days. Stay tuned!

@stakx
Copy link
Contributor

stakx commented Dec 22, 2018

I haven't done any real analysis yet, but I've tracked this down to commit c8c5db1.

@stakx
Copy link
Contributor

stakx commented Dec 22, 2018

Actually looking at the commit I've referenced, things look quite simple: Before that commit (which was the last commit before v4.7.7 got released), Moq's .Protected() facility matched all parameters by their precise types; afterwards, it matched them by assignment compatibility—which, in your case, leads to two matches.

The reason why exactParameterMatch: true didn't work in your case is because you never actually called a method with such a parameter. At present, there are the following overloads in that method group:

  • ISetup<TMock> Setup(string voidMethodName, params object[] args)
  • ISetup<TMock, TResult> Setup<TResult>(string methodOrPropertyName, params object[] args)
  • ISetup<TMock, TResult> Setup<TResult>(string methodOrPropertyName, bool exactParameterMatch, params object[] args)

Since you're setting up a method with a return type of void, the compiler picked the first of these three overloads and your second argument true (which you thought was exactParameterMatch) got used as the first value in the args array.

We should add a new overload ISetup<TMock> Setup(string voidMethodName, bool exactParameterMatch, params object[] args)to Moq to fix this issue.

@stakx
Copy link
Contributor

stakx commented Dec 22, 2018

@sdepouw - Would you be interested in contributing this addition?

(If so, here's a rough summary of what would need to be done:

  • Add all of the above regression tests to the test project, e.g. as a new class Issue735 in test/Moq.Tests/Regressions/IssueReportsFixture.cs.
  • Define the new overload in src/Moq/Protected/IProtectedMock.cs and implement it in src/Moq/Protected/ProtectedMock.cs.
  • Add two entries to CHANGELOG.md, one under Unreleased > Added for the new API method, the other under Unreleased > Fixed referencing this issue.)

@stakx stakx added this to the 4.11.0 milestone Dec 22, 2018
@stakx
Copy link
Contributor

stakx commented Dec 22, 2018

Btw. if you need another workaround that doesn't require you to change MyAbstractClass, here it is:

// In your test project, define an interface describing the protected members of your actual class.
// Note that your actual class does *not* need to implement this:
interface IMyAbstractClassProtectedMembers
{
    void ApplyRule(IDictionary<string, object> tokens);
}

// Then use that interface as follows to set up `MyAbstractClass.ApplyRule`:
var mockedRule = new Mock<MyAbstractClass>();
mockedRule
    .Protected().As<IMyAbstractClassProtectedMembers>()
    .Setup(m => m.ApplyRule(It.IsAny<IDictionary<string, object>>()))
    .Callback(() => _timesApplyRuleCalled++);

// (The rest of your code stays the same.)

(.Protected().As<T>() was added back in v4.8.0.)

@Shereef
Copy link
Contributor

Shereef commented Feb 19, 2019

I have the same issue with SubClass : ParentClass where SubClass has method Populate which overrides Populate on ParentClass and I get the same exact exception

Test Name:    SubClassPopulateCalledTest
Test FullName:    SubClassPopulateCalledTest
Test Source:    D:\Models\Ticket\Populate_BaseTests.cs : line 303
Test Outcome:    Failed
Test Duration:    0:01:34.727

Result StackTrace:    
at System.Linq.Enumerable.SingleOrDefault[TSource](IEnumerable`1 source, Func`2 predicate)
  at Moq.Protected.ProtectedMock`1.GetMethod(String methodName, Boolean exact, Object[] args) in C:\projects\moq4\src\Moq\Protected\ProtectedMock.cs:line 218
  at Moq.Protected.ProtectedMock`1.GetMethod(String methodName, Object[] args) in C:\projects\moq4\src\Moq\Protected\ProtectedMock.cs:line 212
  at Moq.Protected.ProtectedMock`1.Verify(String methodName, Times times, Object[] args) in C:\projects\moq4\src\Moq\Protected\ProtectedMock.cs:line 146
  at PosOnlineWebService.Tests.Models.Ticket.Populate_BaseTests.SubClassPopulateCalledTest() in D:\Dev Online\DSICollection\LiveAccess2\PosOnlineWebService.Tests\Models\Ticket\Populate_BaseTests.cs:line 305
Result Message:    System.InvalidOperationException : Sequence contains more than one matching element
running :               mockedSubClassModel.Protected().Verify("Populate", Times.Once(), ItExpr.Ref<SubClassDTO>.IsAny);

I am trying @stakx's work around but it's not working with our setup for some reason, probably my fault, I'll update here if I get it to work.

@stakx
Copy link
Contributor

stakx commented Feb 20, 2019

@Shereef, if you have a small repro code example, feel free to share it. Might look like the same problem, but have different causes.

@Shereef
Copy link
Contributor

Shereef commented Feb 20, 2019

@stakx: the problem is that GetMethod is looking for compatible params and my params inherit from eachother and so the parent of the class has a Populate with 1 param that is the parent of the subclass Populate param so it's compatible so SingleOrDefault throws the exception mentioned above.

I don't have an example readily available but I have done my research and this is I have come up with.

Until this but is fixed I am using inheriting Mock and adding thise method:

        public void ProtectedVerify(string methodName, int times) // Should check parameters in the future
        {
            System.Collections.Generic.IEnumerable<IInvocation> methodInvocations = Invocations.Where(i => methodName.Equals(i.Method.Name));
            int actual = methodInvocations.Count();
            Assert.AreEqual(times, actual, $"{methodName} was supposed to be invoked {times} times but was invoked {actual} times.");
        }

It doesn't verify the arguments but my case doesn't require that.

Shereef added a commit to Shereef/moq4 that referenced this issue Feb 20, 2019
Shereef added a commit to Shereef/moq4 that referenced this issue Feb 20, 2019
@Shereef
Copy link
Contributor

Shereef commented Feb 20, 2019

@stakx I didn't know it would show up here but my commit on my fork adds test cases for it Shereef@9aee4d2

please clone and run it, I wanted to do it cleanly but I had to add a nuget to access the protected method because I didn't know how to otherwise do it without that nuget

I hope this helps, P.S. I pulled latest and this test still doesn't pass

It fails on the setup and if we make call base true it will fail on the verify too

Shereef added a commit to Shereef/moq4 that referenced this issue Feb 20, 2019
@Shereef
Copy link
Contributor

Shereef commented Feb 20, 2019

Added another case that actually breaks on Verify

Hope this helps!

@stakx stakx removed their assignment Feb 20, 2019
@stakx
Copy link
Contributor

stakx commented Feb 20, 2019

@Shereef - Thanks for the pointer to your tests!

Regarding Shereef@9aee4d2:

I didn't know how to otherwise do it without that nuget

Using that NuGet package is one option, but since we're in a test setting where we have control over (most of) our test types, it is probably easier to simply add public proxy methods to the test types that invoke their protected counterparts:

 public class Parent
 {
-    protected bool Populate(ref ParentDto dto)
+    protected virtual bool Populate(ref ParentDto dto)  // see comment below
     {
         return true;
     }
+
+    public bool InvokePopulate(ref ParentDto dto)
+    {
+        return this.Populate(ref dto);
+    }
 }

 public class Child : Parent
 {
-    protected bool Populate(ref ChildDto dto)
+    protected virtual bool Populate(ref ChildDto dto)  // see comment below
     {
         return true;
     }
+
+    public bool InvokePopulate(ref ChildDto dto)
+    {
+        return this.Populate(ref dto);
+    }
 }

Then we can simply do this:

-Microsoft.VisualStudio.TestTools.UnitTesting.PrivateObject po = new Microsoft.VisualStudio.TestTools.UnitTesting.PrivateObject(mock.Object);
-_ = po.Invoke("Populate", dto);
+mock.Object.InvokePopulate(ref dto);

Since these public methods are non-virtual, they are unproxyable for Moq and therefore won't be recorded as invocations; so there's no danger that they'll affect the test result.

Note that for the same reason just mentioned, your protected methods should be declared virtual. Otherwise Moq won't be able to proxy nor record them.

I hope this helps

It does, thank you! One problem is that we've fixed the Setup situation by adding a new overload with an exactParameterMatch parameter, but we haven't yet done the same for the Verify method group.

https://github.com/moq/moq4/blob/b60c0d93e295be015bb71f59451f31a50e35262d/src/Moq/Protected/ProtectedMock.cs#L157-L166

The Verify methods will currently never perform an exactParameterMatch: true, which appears to be the problem in your scenario. I'll open a new issue to look into this.

I'll look at your second test shortly.

@stakx
Copy link
Contributor

stakx commented Feb 20, 2019

Added another case that actually breaks on Verify

@Shereef, your second test (Shereef@27ca84b) fails for the same reason. Once there is a Verify overload allowing you to specify exactParameterMatch: true, things should work.

@stakx
Copy link
Contributor

stakx commented Feb 20, 2019

I've opened an issue about this (#752), so if you'd like to add any more information or test cases, I suggest we continue this discussion over there.

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

Successfully merging a pull request may close this issue.

3 participants