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

Fixed issue #314 #704

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions build_all_vs2017.bat
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
@echo off
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 this batch file and the vswhere.exe binary, for two reasons: (1) I recently took pains to get rid of unnecessary build scripts, I wouldn't want to go back to a batch file. (2) Fixing the build is unrelated to the issue you're trying to solve.

If you have problems building Moq, then please raise a new issue so we can look at it.

set CONFIG=Debug
set VisualStudioVersion=15.0

for /f "usebackq tokens=*" %%i in (`vswhere -latest -requires Microsoft.Component.MSBuild -property installationPath`) do (
set INSTALLDIR=%%i
)

"%INSTALLDIR%\MSBuild\%VisualStudioVersion%\bin\MSBuild.exe" ".\moq.sln" /t:restore /t:build

if not %ERRORLEVEL%==0 goto fail

echo %time%
echo.
color

pause
exit /b 0

:end

:fail
color 07

echo.
echo.
echo Failed
echo.
pause
exit /b 1
9 changes: 7 additions & 2 deletions src/Moq/Extensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,11 @@ public static bool IsPropertyIndexerSetter(this MethodInfo method)
return method.Name.StartsWith("set_Item", StringComparison.Ordinal);
}

public static bool IsPropertyIndexer(this MethodInfo method)
{
return method.IsPropertyIndexerGetter() || method.IsPropertyIndexerSetter();
}

public static bool IsPropertySetter(this MethodInfo method)
{
return method.Name.StartsWith("set_", StringComparison.Ordinal);
Expand Down Expand Up @@ -91,7 +96,7 @@ public static void ThrowIfNotMockeable(this MemberExpression memberAccess)

public static bool IsMockeable(this Type typeToMock)
{
// A value type does not match any of these three
// A value type does not match any of these three
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert this white space-only change.

// condition and therefore returns false.
return typeToMock.GetTypeInfo().IsInterface || typeToMock.GetTypeInfo().IsAbstract || typeToMock.IsDelegate() || (typeToMock.GetTypeInfo().IsClass && !typeToMock.GetTypeInfo().IsSealed);
}
Expand Down Expand Up @@ -293,4 +298,4 @@ private static void AddPropertiesInDepthFirstOrderTo(this Type type, List<Proper
}
}
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert this line separator-only change.

24 changes: 13 additions & 11 deletions src/Moq/FluentMockVisitor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -100,10 +100,10 @@ private MethodInfo GetTargetMethod(Type objectType, Type returnType)
{
//.Setup(mock => mock.Solution)
return typeof(Mock<>)
.MakeGenericType(objectType)
.GetMethods("Setup")
.First(m => m.IsGenericMethod)
.MakeGenericMethod(returnType);
.MakeGenericType(objectType)
.GetMethods("Setup")
.First(m => m.IsGenericMethod)
.MakeGenericMethod(returnType);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert this white space-only change.

}
else
{
Expand All @@ -115,11 +115,11 @@ private MethodInfo GetTargetMethod(Type objectType, Type returnType)

// Args like: string IFoo (mock => mock.Value)
private Expression TranslateFluent(Type objectType,
Type returnType,
MethodInfo targetMethod,
Expression instance,
ParameterExpression lambdaParam,
Expression lambdaBody)
Type returnType,
MethodInfo targetMethod,
Expression instance,
ParameterExpression lambdaParam,
Expression lambdaBody)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert this white space-only change.

{
var funcType = typeof(Func<,>).MakeGenericType(objectType, returnType);

Expand Down Expand Up @@ -166,7 +166,9 @@ private static Mock<TResult> FluentMock<T, TResult>(Mock<T> mock, Expression<Fun

Mock fluentMock;
MockWithWrappedMockObject innerMock;
if (mock.InnerMocks.TryGetValue(info, out innerMock))

//When property is indexer, the method name is same, in this case i have return other instance of mock.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: i have return other instance of mockignore any previously generated inner mocks, which might have been created for different indexer arguments.

if (!info.IsPropertyIndexer() && mock.InnerMocks.TryGetValue(info, out innerMock))
Copy link
Contributor

Choose a reason for hiding this comment

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

This change fixes the tests you added, but unfortunately it breaks a related scenario:

public interface IA
{
	IB this[int index] { get; }
}

public interface IB
{
	string X { get; }
	string Y { get; }
}

[Fact]
public void Fixed_by_this_change()
{
    var mock = Mock.Of<IA>(m => m[0].X == "0X" && m[1].X == "1X");
    Assert.Equal("0X", mock[0].X);
    Assert.Equal("1X", mock[1].X);
}

[Fact]
public void Broken_by_this_change()
{
    var mock = Mock.Of<IA>(m => m[0].X == "0X" && m[0].Y == "0Y");
    Assert.Equal("0X", mock[0].X);
    Assert.Equal("0Y", mock[0].Y);
}

I fear that the problem here is the whole InnerMocks mechanism, which completely ignores any method arguments. (I am currently working on removing InnerMocks and replacing it with regular setups, but I am not quite finished with that change yet.)

{
fluentMock = innerMock.Mock;
}
Expand All @@ -191,4 +193,4 @@ private static Mock<TResult> FluentMock<T, TResult>(Mock<T> mock, Expression<Fun
return (Mock<TResult>)fluentMock;
}
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert this line separator-only change.

35 changes: 35 additions & 0 deletions tests/Moq.Tests/Regressions/FluentMockVisitorIssues.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
using Xunit;
Copy link
Contributor

@stakx stakx Oct 11, 2018

Choose a reason for hiding this comment

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

Right now, we have one single file for all regression tests coming from GitHub issues; IssueReportsFixture.cs. Usually, there's a #region <IssueNumber> for every issue (they are sorted by source & issue number), and in that region there's a public class Issue<IssueNumber> containing all tests related to that issue.

Could you please move these tests there? Thanks!

(Btw., the following hint is not relevant if you move your tests to IssueReportsFixture.cs, but if you create a new code file anywhere in the Moq solution, you should also add the copyright notice at the very beginning of the new file. You can usually just copy & paste it from any other code file:)

+// Copyright (c) 2007, Clarius Consulting, Manas Technology Solutions, InSTEDD.
+// All rights reserved. Licensed under the BSD 3-Clause License; see License.txt.
+


namespace Moq.Tests.Regressions
{
public class FluentMockVisitorIssues
{
[Fact]
public void WhenQueryingWithTwoIndexes_ThenSetsThemDirectly1()
{
var foo = Mock.Of<IFoo>(x => x[0].Value == "hello" && x[1].Value == "goodbye");

Assert.Equal("hello", foo[0].Value); // Fails here as foo[0] is the same object as foo[1] and foo[1].Value == "goodbye", Fixed!
Copy link
Contributor

Choose a reason for hiding this comment

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

The "fails here" comment will no longer make sense once you've fixed the issue. Please remove it. (With all tests, the basic assumption is that they will pass, so there should be no comment saying "this fails". ;-)

Assert.Equal("goodbye", foo[1].Value);
}

[Fact]
public void WhenQueryingWithTwoIndexes_ThenSetsThemDirectly2()
{
var foo = Mock.Of<IFoo>(x => x[0] == Mock.Of<IBar>(b => b.Value == "hello") && x[1] == Mock.Of<IBar>(b => b.Value == "goodbye"));

Assert.Equal("hello", foo[0].Value); // These pass no problem
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar as in the above comment, there is no need to say "these pass no problem" — that is what we assume for all tests, anyway.

(I know you copy & pasted these tests from the issue, and that is perfectly fine. Just pointing out that they often need to be changed a tiny bit so that they make sense in their new context.)

Assert.Equal("goodbye", foo[1].Value);
}

public interface IFoo
{
IBar this[int index] { get; }
}

public interface IBar
{
string Value { get; }
}
}
}
Binary file added vswhere.exe
Binary file not shown.