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

Extracting Func<T, bool> when using It.Is<T> can result in missing method setups #374

Closed
Aerendel opened this issue Jun 15, 2017 · 3 comments

Comments

@Aerendel
Copy link

Scenario

Extracting (as a variable) the Func<T, bool> part of the expression parameter of It.Is<T> can result in missing setups when performing more than one setup on the same method using the same lambda parameter name.

Illustration

This passes

[Fact]
public void TestExtractedFunc_Passes()
{
    //Arrange
    Mock<ITestObject> testObj = new Mock<ITestObject>();
    
    Func<string, bool> matchPing = s => s == "Ping";
    Func<string, bool> matchPong = s => s == "Pong";

    testObj.Setup(p => p.Submit(It.Is<string>(t => matchPing(t)))).Returns("Pong");
    testObj.Setup(p => p.Submit(It.Is<string>(differentName => matchPong(differentName)))).Returns("Ping");

    //Assert
    Assert.Equal("Pong", testObj.Object.Submit("Ping"));
    Assert.Equal("Ping", testObj.Object.Submit("Pong"));
}

This does not pass (returned value is null)

[Fact]
public void TestExtractedFunc_DoesNotPass()
{
  //Arrange
  Mock<ITestObject> testObj = new Mock<ITestObject>();

  Func<string, bool> matchPing = s => s == "Ping";
  Func<string, bool> matchPong = s => s == "Pong";

  testObj.Setup(p => p.Submit(It.Is<string>(sameName => matchPing(sameName)))).Returns("Pong");
  testObj.Setup(p => p.Submit(It.Is<string>(sameName => matchPong(sameName)))).Returns("Ping");

  //Assert
  Assert.Equal("Pong", testObj.Object.Submit("Ping"));
  Assert.Equal("Ping", testObj.Object.Submit("Pong"));
}

Implications

This is not a shower stopper since there are alternatives to extracting the Func<T, bool> such as extracting the full Expression, using custom Matcher functions or of course using different parameter names as demonstrated. However, extracting the Func<> is one the most obvious and easy way to pass anonymous functions to It.Is when the method body needs more than 1 statement. This can be a fairly common scenario when testing method calls thoroughly, and troubleshooting something like can turn into a huge time sink.

Root cause

A string representation of the expression passed into It.Is is used by Moq to generate an ExpressionKey to store the Setups in a dictionary of method calls. This happens in In method Moq.Interceptor.AddCall(IProxyCall, SetupKind) of https://github.com/moq/moq4/blob/master/Source/Interceptor.cs. Unfortunately the conversion from expression body to a string based key loses reference to the named Func, resulting in both setups having identical keys.

I have included a full repro class + more comments/details in this gist: https://gist.github.com/Aerendel/2ead83066408764b954a74bf4f1c71e1

Note: I am nowhere close to having a fix for this unfortunately, as it requires diving deep into the Expression evaluation code, or understanding why the expression body is being transformed/simplified/stringified before building the key in the first place. Hopefully someone with familiarity around this can propose a fix faster.

@stakx
Copy link
Contributor

stakx commented Jun 15, 2017

Thanks for reporting this, and for the analysis. I have some good news for you: Both of your tests pass in the current develop branch. I tracked this down to #363 / commit b4829ff, which fixed some faulty equality comparison logic in ExpressionKey. This will be in the next Moq release (version >4.7.25), which is probably only a couple of days away now.

If you'd like, feel free to submit a pull request that adds your above two tests and the ITestObject interface not shown as a separate #region 374 with a nested public class Issue374 to UnitTests/Regressions/IssueReportsFixture.cs. I'll be happy to merge them in. Always good to have tests. ;)

@Aerendel
Copy link
Author

Thanks for the reply. I guess that will teach me to check against the develop branch before reporting issues... 🤦‍♂️

@stakx
Copy link
Contributor

stakx commented Jun 15, 2017

That's no problem at all... though you probably invested a fair amount of time. Thanks for that! :)

Note that this issue should get closed automatically once the fix is merged into master (which is also when a new NuGet package will get published).

@stakx stakx closed this as completed Jun 18, 2017
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