Update from 4.13.1 to 4.16.1 lazy evaluation setups fail

[b3go]

Hi!

I have a few tests that didn't work anymore after update to the current version. These tests were not written by me so I can't tell you much about why they were written like that.

1. Lazy evaluation of setups
   In the TestIntialize method there are 2 Setups for the same method but depending on the argument it should return another value. Initially patternKey and exeKey are both string.Empty.

            this.settingsServiceMock.Setup(x => x.GetSetting(It.Is<string>((y) => y == this.patternKey))).Returns(() => this.patternKey);
            this.settingsServiceMock.Setup(x => x.GetSetting(It.Is<string>((y) => y == this.exeKey))).Returns(() => this.exeKey);

In the test method however patternKey and exeKey get changed to different strings. Bevore the GetSettings method on the mock is called. Before the update a breakpoint inside the It.Is expression was hit after calling GetSettings but now it doesn't get hit anymore. I guess the evaluation of the It.is was changed so the initial value is considered. Shouldn't that be considered a breaking change? Maybe you could extend the documentation in that case. I fixed it for my case like that:

            this.settingsServiceMock.Setup(x => x.GetSetting(It.IsAny<string>())).Returns<string>(s =>
            {
                if (s == this.patternKey)
                    return this.patternKey;
                return s == this.exeKey ? this.exeKey : null;
            });

2. Setup generic methods with derived interfaces
   The test setups a generic method twice, first with the derived interface and then with the base interface and for either setup it returns differnt object. Before the update it worked like that without issues but after the update only the setup for the base interface was used. After I changed the registration order, to first setup with the base interface and then with the derived interface, it again worked like a charm. I guessed that the setup with the base interface somehow overwrites the setup with the derived one. Is this intended?

tl;dr I was able to fix my problems, still would be nice if I could get some feedback to these issues to better understand what happens.

[rjvdboon]

I think I'm hit by the same issue after upgrading to 4.16. @b3go I have applied your workaround successfully, thanks for finding that out!

[stakx]

Hi @b3go. You've reported two problems in one issue; this has the potential to get confusing quickly once we start discussing things, so I'd like to ask you to move one of those problems (say, the 2nd one) to a separate GitHub issue.

For now, I am going to focus only on (1). I cannot actually reproduce the problem you've reported. Could you please provide minimal but complete example code that exhibits the problem? Thanks.

[b3go]

Hi @stakx, thx for the response, I'll open another issue for (2).

I wrote a quick example which shows the difference in 4.13.1 and 4.16.1.

void Main()
{
	var patternKey = string.Empty;
	var exeKey = string.Empty;

	var mock = new Mock<ISettingsService>();
	mock.Setup(x => x.GetSetting(It.Is<string>((y) => y == patternKey))).Returns(() => patternKey);
	mock.Setup(x => x.GetSetting(It.Is<string>((y) => y == exeKey))).Returns(() => exeKey);

	patternKey = "foo";
	exeKey = "bar";

 	// returns "foo" in 4.13.1 but null in 4.16.1
	var result = mock.Object.GetSetting(patternKey);
        Console.WriteLine(result);

 	// returns "bar" in 4.13.1 and 4.16.1
	result = mock.Object.GetSetting(exeKey);
        Console.WriteLine(result);
}

public interface ISettingsService
{
	string GetSetting(string key);
}

It seems the second setup overwrites the first, because the It.Is expression is handled as if it was the same. Anyway I like the syntax GetSettings(It.IsAny()).Returns(...) way more than what the other dev used. I just want to let you know there was a breaking change, somewhere between 4.13.1 and 4.16.1, which doesn't show up in the documentation.

Thank you for your work!

[stakx]

OK, this regression appears to be caused by #1081 (which in turn was a fix for #1054). This PR makes it so that whenever Moq compares two LINQ expression trees, it evaluates captured variables so that their values will be compared (and not their identities).

I'm not sure I have the full picture just yet, but to state the problem in a simplified form, evaluating captured variables only makes sense at invocation time, when concrete argument values are to be compared against captured variables in a setup's expression. However, it does not make sense at setup time, when two setup expressions are compared against one another in order to determine whether a new setup shadows an existing one. In this latter case, the identities of captured variables matter, so they shouldn't be evaluated.

I'm currently trying to figure out how exactly to fix this without regressing against #1054. I suspect this will involve treating Quote-d expressions as a barrier to captured variable evaluation.