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

Mock<T>.CallBase seems to be broken #640

Closed
fgreinacher opened this issue Jul 13, 2018 · 10 comments · Fixed by #641
Closed

Mock<T>.CallBase seems to be broken #640

fgreinacher opened this issue Jul 13, 2018 · 10 comments · Fixed by #641
Labels
Milestone

Comments

@fgreinacher
Copy link

fgreinacher commented Jul 13, 2018

We are currently trying to upgrade to Moq 4.8.2 and we found several test failures around partial mocking of classes. Here is the repro:

internal interface Interface
{
    int Method();
}

internal class Class : Interface
{
    public int Method() => 42;
}

class CallBaseTests
{
    [Test]
    public void Fails()
    {
        var classMock = new Mock<Class> { CallBase = true };
        var interfaceMock = classMock.As<Interface>();
        interfaceMock.CallBase = true;

        Assert.That(interfaceMock.Object.Method(), Is.EqualTo(42));
    }

    [Test]
    public void Succeeds()
    {
        var classMock = new Mock<Class> { CallBase = true };
        var interfaceMock = classMock.As<Interface>();
        interfaceMock.Setup(x => x.Method()).CallBase();

        Assert.That(interfaceMock.Object.Method(), Is.EqualTo(42));
    }
}

I would expect that setting the CallBase property of the mock to true is the same as calling SetUp().CallBase for every method. Is this assumption wrong or is this a regression?

Update: Making the interface public fixes the problem (but that's no option for us)

public interface Interface
{
    int Method();
}
@stakx
Copy link
Contributor

stakx commented Jul 13, 2018

We are currently trying to upgrade to Moq 4.2.1 [...].

Did you mean "upgrade to", or "upgrade from"?

(If the former, please upgrade to the latest version, 4.8.3.)

@fgreinacher
Copy link
Author

Sorry, I mixed up Castle.Core and Moq versions. We want 4.8.2, but I just reproduced this with 4.8.3.

@fgreinacher
Copy link
Author

@stakx
Copy link
Contributor

stakx commented Jul 13, 2018

@fgreinacher - Thanks for providing a precise repro. I've been able to determine what's going on. The TL;DR version is that this is an old but bug that has only become noticeable after another old bug was fixed. I'm going to fix it. Read on if you're interested in the details.

The problem you've noticed is the result of two aspects that have changed over time:

  1. Between Moq versions 4.2.1402.2112 and 4.2.1408.619 (in 6ad7103), a change was made that meant Moq would ignore a type's implemented interfaces unless they are declared public. The rationale behind this was that if one didn't include the [assembly: InternalsVisibleTo("DynamicProxyGenAssembly2")] bit, then mocking a type such as your Class would fail because DynamicProxy couldn't access the inaccessible Interface.

    As a consequence of that change, because your Class implements Interface which is declared non-public, Moq ignores that type relationship and, to put it simply, it "doesn't see" the base method, so no CallBase happens.

  2. While Moq doesn't "see" the type relationship between Class and Interface, it is of course still there. So if Moq were to perform the CallBase, it would work and both your tests would pass. And this was actually the case before version 4.8.0-rc1! The logic that decides whether the base method should be called or not used to be cryptic in the extreme, so I decided to rewrite it in 5837c53 and 7e12f34 to be less convoluted.

    It appears that my rewrite got rid of an bug that I didn't even notice (like I said, the code used to be cryptic): Moq would blindly do the CallBase even though it was supposed to not see the base method from the non-public base interface. Now that the rewrite fixed this, the CallBase would no longer happen (as it should be).

In other words, the code rewrite mentioned in (2) has uncovered an old shortcoming of (1). The fix required here is this: Instead of just looking at the interface's declared visibility, we should really be asking DynamicProxy, "Is this base interface visible to you?" If yes, let Moq see it; otherwise, exclude it.

@stakx
Copy link
Contributor

stakx commented Jul 13, 2018

@fgreinacher - This should be fixed in the next version of Moq (4.9.0), which I have just published on NuGet.

@fgreinacher
Copy link
Author

Thanks @stakx, I’ll test our real code with the new version on Monday. Thanks also for the detailed explanation!

@stakx
Copy link
Contributor

stakx commented Jul 13, 2018

No worries. Please let me know whether 4.9.0 resolves your problem, or whether there are any remaining issues.

@fgreinacher
Copy link
Author

fgreinacher commented Jul 16, 2018

@stakx Any way to get a „hotfix“ Release based on Moq 4.8.2 (something like 4.8.2.1)? We have to stick with Castle.Core 4.2.1 for process reasons and Moq 4.9 depends on a later Castle.Core version.

@stakx
Copy link
Contributor

stakx commented Jul 16, 2018

@fgreinacher - I would really like to avoid doing such a hotfix release. If there were more Moq users with the same problem (not being able to upgrade to the latest Castle Core) I'd consider it, but if it's just you, you're perhaps better off working with a private fork of Moq where you downgrade Castle Core... or do some MSBuild fiddling where you replace the newer Castle Core version with the one you need.

Is any of that feasible for you?

@fgreinacher
Copy link
Author

Sure, that's fine. I forked from https://github.com/moq/moq4/releases/tag/v4.8.2 and backported your fix to https://github.com/fgreinacher/moq4/tree/v4.8.4 from which we'll build our private 4.8.4 👍

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

Successfully merging a pull request may close this issue.

2 participants