Fixed issue #314 #704
Fixed issue #314 #704
Conversation
| @@ -0,0 +1,30 @@ | |||
| @echo off | |||
There was a problem hiding this comment.
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.
| @@ -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 | |||
There was a problem hiding this comment.
Please revert this white space-only change.
| @@ -293,4 +298,4 @@ private static void AddPropertiesInDepthFirstOrderTo(this Type type, List<Proper | |||
| } | |||
| } | |||
| } | |||
| } | |||
| } | |||
There was a problem hiding this comment.
Please revert this line separator-only change.
| .MakeGenericType(objectType) | ||
| .GetMethods("Setup") | ||
| .First(m => m.IsGenericMethod) | ||
| .MakeGenericMethod(returnType); |
There was a problem hiding this comment.
Please revert this white space-only change.
| MethodInfo targetMethod, | ||
| Expression instance, | ||
| ParameterExpression lambdaParam, | ||
| Expression lambdaBody) |
There was a problem hiding this comment.
Please revert this white space-only change.
| @@ -191,4 +193,4 @@ private MethodInfo GetTargetMethod(Type objectType, Type returnType) | |||
| return (Mock<TResult>)fluentMock; | |||
| } | |||
| } | |||
| } | |||
| } | |||
There was a problem hiding this comment.
Please revert this line separator-only change.
| @@ -0,0 +1,35 @@ | |||
| using Xunit; | |||
There was a problem hiding this comment.
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.
+| { | ||
| 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! |
There was a problem hiding this comment.
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". ;-)
| { | ||
| 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 |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
I had a look at the actual bug fix, well done on finding one way of fixing it! I had a closer look and noticed that this change breaks another very related scenario, so unfortunately we're not quite there yet. :-/
Also, could you please give this PR a more descriptive title instead of just referring to the issue number you're fixing? When PRs get merged, their titles are included in the commit message. It would be good to have a small summary of what the PR does there.
| 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. | ||
| if (!info.IsPropertyIndexer() && mock.InnerMocks.TryGetValue(info, out innerMock)) |
There was a problem hiding this comment.
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.)
| @@ -166,7 +166,9 @@ private MethodInfo GetTargetMethod(Type objectType, Type returnType) | |||
|
|
|||
| 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. | |||
There was a problem hiding this comment.
Suggestion: i have return other instance of mock → ignore any previously generated inner mocks, which might have been created for different indexer arguments.
|
Closing this (as per Gitter chat between the PR author and me). We'll reopen this once the restrictive |
|
@damianpumar - After your Gitter ping I've quickly gone through your previous contributions (this one) to see if there's anything we could pick up again, and then I saw that this issue has since been fixed as a direct consequence of replacing the old |
Added .bat compilation, because vs 2017 have many problems to build sln.
Fixed issue Setting multiple indexed objects' property directly via Linq fails #314
Added unit test of example explained in Setting multiple indexed objects' property directly via Linq fails #314