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

Protected Verify doesn't accept mocks of interfaces as valid for those interfaces #334

Closed
MatKubicki opened this issue Mar 21, 2017 · 3 comments

Comments

@MatKubicki
Copy link
Contributor

I'm not sure what change has caused this problem, we've skipped a few mock versions now waiting for various problems to be solved and at sometime between those this has broken.

In short here is a new unit test for Moq that should pass, but fails:

public class IssueXX
{
    [Fact()]
    public void ProtectedVerifyDoesntAcceptMocksForInterfaces()
    {
        Mock<SomeClassToTest> testedClass = new Mock<SomeClassToTest>();
        Mock<IParamType> input = new Mock<IParamType>();

        testedClass.Object.DoWork(input.Object);
        testedClass.Protected().Verify("Protected", Times.Once(), input.Object);
    }
    public abstract class SomeClassToTest
    {
        public void DoWork(IParamType input)
        {
            Protected(input);
        }
        protected abstract void Protected(IParamType input);
    }
    public interface IParamType
    {
    }
}

The problem seems to be that in Extensions there is a method 'HasMatchingParameterTypes' that does a straight compare of the types of the method parameters and the provided parameters. This fails as where as the method has IParamType, the provided argument is a CastleProxy that happens to implement IParamType.

I don't yet know where the fix for this should go, my first thought was in the HasMatchingParameterTypes method, its only used in Protected Moqs so it seemed safe. But from a quick look into othe changes to Protected it seems liek I should be doing the work in one of the methods in ProtectedMock, for example ToExpressionArg.

I also notice that there is a pull request with some changes to this code floating for the last 12 months. #200
If we can get that merged in as well that might help avoid a merge conflict.

@MatKubicki
Copy link
Contributor Author

MatKubicki commented Mar 22, 2017

For reference the change I was thinking of to HasMatchingParameterTypes was:

public static bool HasMatchingParameterTypes(this MethodInfo method, Type[] paramTypes)
{
	var types = method.GetParameterTypes().ToArray();
	if (types.Length != paramTypes.Length)
	{
		return false;
	}
	for (int i = 0; i < types.Length; i++)
	{
		if (!types[i].IsAssignableFrom(paramTypes[i]))
		{
			return false;
		}
	}
	return true;
}

With the key line being:
if (!types[i].IsAssignableFrom(paramTypes[i]))

It seems to work in that it passes Moqs unit tests and hasn't broken any of our thousands of Moq based unit tests either. Of course all our tests are of a certain 'style' so i'm a bit concerned to change this. Especially as our Protected tests probably don't make use of many ItExpr.Is() type tests.

@jeremymeng
Copy link
Contributor

You are right. I introduced this method when adding .NET Core support. It looks like the behavior doesn't match the old Desktop code thus the bug. It was new code and not much test coverage was added.

According to the reference code, the checking is

                if (par.Length != types.Length)
                    continue;
                for (j=0;j<types.Length;j++) {
                    Type pCls = par[j].ParameterType;
                    if (pCls == types[j])
                        continue;
                    if (pCls == typeof(Object))
                        continue;
                    if (pCls.IsPrimitive) {
                        if (!(types[j].UnderlyingSystemType is RuntimeType) ||
                            !CanConvertPrimitive((RuntimeType)types[j].UnderlyingSystemType,(RuntimeType)pCls.UnderlyingSystemType))
                            break;
                    }
                    else {
                        if (!pCls.IsAssignableFrom(types[j]))
                            break;
                    }

You should submit a PR to fix it. I don't know how easy to check the convertibility of primitives, but even with IsAssignableFrom it is still better.

@MatKubicki
Copy link
Contributor Author

Thanks for getting this fixed so quickly guys.

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

No branches or pull requests

2 participants