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.

Sign up for GitHub

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

Jump to bottom

fix ProtectedMock fails when arguments are of type Expression #1188 #1189

Merged
merged 6 commits into from Aug 3, 2021
Merged

fix ProtectedMock fails when arguments are of type Expression #1188 #1189

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ae588a4
fix #1188
tonyhallett Jul 22, 2021
1059cd7
Add changelog entry
tonyhallett Jul 27, 2021
c145b2e
code style
tonyhallett Aug 2, 2021
3f381ea
Correct LambdaExpression logic and reduce unnecessary Matcher checks
tonyhallett Aug 2, 2021
e05680b
remove condition match test and reorder match test order
tonyhallett Aug 2, 2021
f6a0837
extend the scope of the fix in the changelog
tonyhallett Aug 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Expand Up @@ -13,6 +13,7 @@ The format is loosely based on [Keep a Changelog](http://keepachangelog.com/en/1

#### Fixed

* mock.Protected().SetupSet fails when argument is of type Expression (@tonyhallett, #1189)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove. (We don't need a changelog entry because from a user's perspective, this issue never actually existed: It only surfaced after #1186, but wasn't yet observable before that, i.e. in the latest released version.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it has always been an issue. You changed the issue title from "ProtectedMock fails when arguments are of type Expression" to mock.Protected().SetupSet fails when argument is of type Expression.
I have just put the issue title in to the changelog.

Copy link
Contributor

@stakx stakx Aug 2, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tonyhallett, yes, the issue may always been present, but it never surfaced for Moq users because value was ignored up until recently. (Or am I missing something here?) So users don't need to know about this issue. The PR title is only relevant because it ends up in the commit history. The commit history is important to Moq developers, the changelog OTOH is important to Moq users. Please remove the changelog entry.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue could have surfaced at any time over the last 12? years as ToExpressionArg was being called by ToExpressionArgs, called by the GetMethodCall methods ( args were not ignored ). GetMethodCall methods are called by numerous paths.

For instance
https://github.com/moq/moq4/blob/a6fde8b6d79a7437bf642d115785b97f40779b6a/src/Moq/Protected/ProtectedMock.cs#L47

Link is from before #1186

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I said, perhaps your change to the issue title made you think it only applied to SetupSet which was ignoring the value.

Copy link
Contributor

@stakx stakx Aug 2, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tonyhallet, I see. Thank you for the explanation. The changelog entry certainly made me think so, since it does mention SetupSet (which so far wouldn't have been affected, right? 😉). Let's keep the changelog entry then. Perhaps you can adjust it to include method(s) where this would have been observable even before now? And do feel free to give this PR a more accurate / correct title if you feel that the current title is inaccurate.

* Parameter is invalid in Protected().SetupSet() ... VerifySet (@tonyhallett, #1186)

* Virtual properties and automocking not working for `mock.Protected().As<>()` (@tonyhallett, #1185)
Expand Down
64 changes: 47 additions & 17 deletions src/Moq/Protected/ProtectedMock.cs
View file
Open in desktop
Expand Up @@ -467,26 +467,15 @@ private static Type[] ToArgTypes(object[] args)
{
types[index] = ((MethodCallExpression)expr).Method.ReturnType;
}
else if(ItRefAnyField(expr) is var itRefAnyField && itRefAnyField != null)
stakx marked this conversation as resolved.
Show resolved Hide resolved
{
types[index] = itRefAnyField.FieldType.MakeByRefType();
}
else if (expr.NodeType == ExpressionType.MemberAccess)
{
var member = (MemberExpression)expr;
if (member.Member is FieldInfo field)
{
// Test for special case: `It.Ref<TValue>.IsAny`
if (field.Name == nameof(It.Ref<object>.IsAny))
{
var fieldDeclaringType = field.DeclaringType;
if (fieldDeclaringType.IsGenericType)
{
var fieldDeclaringTypeDefinition = fieldDeclaringType.GetGenericTypeDefinition();
if (fieldDeclaringTypeDefinition == typeof(It.Ref<>))
{
types[index] = field.FieldType.MakeByRefType();
continue;
}
}
}

types[index] = field.FieldType;
}
else if (member.Member is PropertyInfo property)
Expand All @@ -510,16 +499,57 @@ private static Type[] ToArgTypes(object[] args)
return types;
}

private static FieldInfo ItRefAnyField(Expression expr)
{
FieldInfo itRefAnyField = null;

if (expr.NodeType == ExpressionType.MemberAccess)
{
var member = (MemberExpression)expr;
if (member.Member is FieldInfo field)
{
if (field.Name == nameof(It.Ref<object>.IsAny))
{
var fieldDeclaringType = field.DeclaringType;
if (fieldDeclaringType.IsGenericType)
{
var fieldDeclaringTypeDefinition = fieldDeclaringType.GetGenericTypeDefinition();
if (fieldDeclaringTypeDefinition == typeof(It.Ref<>))
{
itRefAnyField = field;
}
}
}
}
}

return itRefAnyField;
}

private static Expression ToExpressionArg(Type type, object arg)
{
if (arg is LambdaExpression lambda)
if (arg is LambdaExpression lambda && !typeof(LambdaExpression).IsAssignableFrom(type))
{
return lambda.Body;
}

if (arg is Expression expression)
{
return expression;
if (!typeof(Expression).IsAssignableFrom(type))
{
return expression;
}

if (expression.IsMatch(out _))
{
return expression;
}

if (ItRefAnyField(expression) != null)
{
return expression;
}
stakx marked this conversation as resolved.
Show resolved Hide resolved

}

return Expression.Constant(arg, type);
Expand Down
25 changes: 24 additions & 1 deletion tests/Moq.Tests/ProtectedMockFixture.cs
View file
Open in desktop
Expand Up @@ -2,7 +2,7 @@
// All rights reserved. Licensed under the BSD 3-Clause License; see License.txt.

using System;

using System.Linq.Expressions;
stakx marked this conversation as resolved.
Show resolved Hide resolved
using Moq.Protected;

using Xunit;
Expand Down Expand Up @@ -855,6 +855,23 @@ public void DoesNotThrowIfVerifySetPropertyTimesReached()
mock.Protected().VerifySet<string>("ProtectedValue", Times.Exactly(2), ItExpr.IsAny<string>());
}

[Fact]
public void SetupShouldWorkWithExpressionTypes()
{
var mock = new Mock<FooBase>();

var expression = Expression.Constant(1);
ConstantExpression setExpression = null;
mock.Protected().SetupSet<ConstantExpression>("ExpressionProperty", expression).Callback(expr => setExpression = expr);
mock.Object.SetExpressionProperty(expression);
Assert.Equal(expression, setExpression);

ConstantExpression setExpression2 = null;
mock.Protected().SetupSet<ConstantExpression>("ExpressionProperty", ItExpr.Is<ConstantExpression>( e => (int)e.Value == 2)).Callback(expr => setExpression2 = expr);
mock.Object.SetExpressionProperty(Expression.Constant(2));
Assert.NotNull(setExpression2);
}

public class MethodOverloads
{
public void ExecuteDo(int a, int b)
Expand Down Expand Up @@ -952,6 +969,12 @@ protected virtual FooBase Overloaded(MyDerived myBase)

public class FooBase
{
protected virtual ConstantExpression ExpressionProperty { get; set; }
stakx marked this conversation as resolved.
Show resolved Hide resolved
public void SetExpressionProperty(ConstantExpression expression)
{
ExpressionProperty = expression;
}

public virtual string PublicValue { get; set; }

protected internal virtual string ProtectedInternalValue { get; set; }
Expand Down