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

Recursive mocks with different paths #946

Closed
danielcweber opened this issue Oct 17, 2019 · 8 comments · Fixed by #948
Closed

Recursive mocks with different paths #946

danielcweber opened this issue Oct 17, 2019 · 8 comments · Fixed by #948
Labels
Milestone

Comments

@danielcweber
Copy link

danielcweber commented Oct 17, 2019

Just a general question: Should this scenario be working at all, or is it not how recursive mocks are meant to be?

var mock = Mock.Of<IRecursiveRoot>(MockBehavior.Loose);

mock.Setup(m => m.Branch.GetLeaf(1).Name).Returns("foo");
mock.Setup(m => m.Branch.GetLeaf(2).Name).Returns("bar");

Assert.Equal("foo", mock.Branch.GetLeaf(1).Name);
Assert.Equal("bar", mock.Branch.GetLeaf(2).Name);

Currently, it seems the second setup of m.Branch completely erases all previous setups, even if previous setups might be reused because actually, there's different paths being setup.

@stakx
Copy link
Contributor

stakx commented Oct 17, 2019

@danielcweber, your example is incomplete, and what's here wouldn't actually compile. I fixed the mistake of mock vs. mock.Object and tried to guess to fill in what IRecursiveRoot might look like. I ended up with this:

+public interface IRecursiveRoot
+{
+    IBranch Branch { get; }
+}

+public interface IBranch
+{
+    ILeaf GetLeaf(int index);
+}

+public interface ILeaf
+{
+    string Name { get; }
+}

-var mock = Mock.Of<IRecursiveRoot>(MockBehavior.Loose);
+var mock = new Mock<IRecursiveRoot>(MockBehavior.Loose);

 mock.Setup(m => m.Branch.GetLeaf(1).Name).Returns("foo");
 mock.Setup(m => m.Branch.GetLeaf(2).Name).Returns("bar");

-Assert.Equal("foo", mock.Branch.GetLeaf(1).Name);
+Assert.Equal("foo", mock.Object.Branch.GetLeaf(1).Name);
-Assert.Equal("bar", mock.Branch.GetLeaf(2).Name);
+Assert.Equal("bar", mock.Object.Branch.GetLeaf(2).Name);

which works just fine since version 4.11.0 where I focused on fixing precisely this class of problems. (In case you are using an older version of Moq, please always check with the latest version before filing an issue.)

it seems the second setup of m.Branch completely erases all previous setups

How did you arrive at this conclusion?

Now to your actual question:

Should this scenario be working at all[?]

Yes, this should work.

@danielcweber
Copy link
Author

Hi @stakx: Gotta admit, I wrote this unit test within the moq v5 solution, then posted the issue here, the difference is of course now apparent to me but I didn't realize. Thanks for clarification, sorry for the confusion. Your take at the interfaces is right, they have also been taken from moq5 unit tests.

I tested it in moq4 and the above indeed works (doesn't in moq5). However, my real-life samples fail. I will have to come up with a more complete repro then.

@danielcweber
Copy link
Author

danielcweber commented Oct 18, 2019

Alright, I was able to come up with a minimal repro for the behaviour I'm observing. Find it here. The key for the repro is

  • params[] int in "Branch"
  • a chain of at least 2 methods ("Branch" and "LeafX").

If you take away one of the factors it works, i.e. params[] arguments seem to work, and chains of 2 methods without params seem to work as well.

@danielcweber
Copy link
Author

danielcweber commented Oct 18, 2019

Have been debugging this a bit, the second setup doesn't override the first but rather coexists, although it should match the first setup (at least for the "Branch"-call), then add a second setup to the continuation. Instead, the second setup ("Branch") matches (because of the lookup in reverse), but the call to "Leaf" then won't.

@stakx
Copy link
Contributor

stakx commented Oct 18, 2019

mock.Setup(m => m.Branch(1).Leaf1()).Returns("foo");
mock.Setup(m => m.Branch(1).Leaf2()).Returns("bar");

the second setup doesn't override the first but rather coexists

This is because the compiler silently converts the 1 to int[] arrays due to the parameter being declared params. I suspect that the compiler creates distinct new int[] { 1 } instances for each call, and all those instances don't test equal due to arrays being reference types. (It doesn't matter that the two arrays have equivalent elements... Moq honors the usual .NET equality conventions, i.e. it compares arguments using a call to Equals.)

Let's see if that theory is correct by passing the same int[] everywhere, instead of letting the compiler produce them:

+var one = new[] { 1 };
+
-mock.Setup(m => m.Branch(1).Leaf1()).Returns("foo");
+mock.Setup(m => m.Branch(one).Leaf1()).Returns("foo");
-mock.Setup(m => m.Branch(1).Leaf2()).Returns("bar");
+mock.Setup(m => m.Branch(one).Leaf2()).Returns("bar");

-Assert.Equal("foo", mock.Object.Branch(1).Leaf1());
+Assert.Equal("foo", mock.Object.Branch(one).Leaf1());
-Assert.Equal("bar", mock.Object.Branch(1).Leaf2());
+Assert.Equal("bar", mock.Object.Branch(one).Leaf2());

... and for sure, this time your test will pass.

@danielcweber
Copy link
Author

It will for sure, but Moq DOES recognize different params-arrays on none-recursive calls. It'll compare these arrays element-by-element. Only on a recursive call it fails to re-recognize the previous setup.

@stakx
Copy link
Contributor

stakx commented Oct 18, 2019

Actually, perhaps this is a bug. Moq uses a dedicated argument matcher for params parameters. Which might mean that for those parameters, Moq should not compare the array references using Equals, but their contents. Will investigate this in some more detail.

@danielcweber
Copy link
Author

Actually, perhaps this is a bug

Yep, for looking up a previous setup, Moq ends up in ExpressionComparer.EqualsConstant, which will not compare arrays. So it's about using the ParamArrayMatcher you mentioned along the way.

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