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

Issue comparing Expressions when one value is a constant #1054

Closed
QTom01 opened this issue Sep 7, 2020 · 2 comments · Fixed by #1081
Closed

Issue comparing Expressions when one value is a constant #1054

QTom01 opened this issue Sep 7, 2020 · 2 comments · Fixed by #1081
Assignees
Milestone

Comments

@QTom01
Copy link

QTom01 commented Sep 7, 2020

I have discovered an issue when using Expressions in a Setup on a mock object.

It seems using a simple comparison Expression fails to match properly when comparing to a string constant.

The below code should reproduce it:

[Test]
public async Task ExpressionTest()
{
    var s1 = "example";

    // Does not work
    const string s2 = "example";

    // Works
    //var s2 = "example";

    var mock = new Mock<IMockObject>();

    mock.Setup(x => x.Method(y => y.Id == s1))
        .ReturnsAsync("mockResult");

    var result = await mock.Object.Method(x => x.Id == s2);
    // Exprect result to be "mockResult", instead it is null

    Assert.AreEqual(result, "mockResult");
}

public class MyObject
{
    public string Id { get; set; }
}

public interface IMockObject
{
    public Task<string> Method(Expression<Func<MyObject, bool>> expression);
}

The two strings s1 and s2 are the same, however when one of them is a constant then it fails to match the two expressions.

I would expect it should work the same when the string is a constant?

This is on a .NET Core 3.1 test project using Moq 4.14.5 and NUnit 3.12.

@QTom01
Copy link
Author

QTom01 commented Sep 7, 2020

For now I can work around by simply assigning a local variable in the test from the constant and using that instead.

@stakx
Copy link
Contributor

stakx commented Sep 7, 2020

Referencing a constant vs. a variable results in slightly different LINQ expression trees: in the former case, the string value gets embedded as a constant, while in the latter case, the variable gets captured in the form of a reference to an anonymous object's field.

We've had a few similar issues in the past, and the approach followed most recently was that captured variables should be evaluated down to a constant value at the time when two LINQ expression trees get compared.

If we applied this rule here, this present issue would be resolved.

However, at present I am not sure whether this rule should apply in this case. The added complication here is that we have a setup LINQ expression tree that embeds another LINQ expression tree (the argument passed to Method)... and we need to know whether we're allowed to "touch" (here: partially reduce) embedded expression trees or whether we should treat them as constants and always leave them unmodified.

This is probably related to the two principal ways how expression trees can reference other expression trees: Expression.Constant and Expression.Quote. I'm not positively certain that Moq makes this distinction correctly in all situations.

TL;DR: I'll look into it, but this will take some thought and time.

@stakx stakx self-assigned this Sep 7, 2020
@stakx stakx added this to the 4.15.0 milestone Oct 26, 2020
mburumaxwell pushed a commit to faluapp/falu-dotnet that referenced this issue Jun 12, 2021
Bumps [Moq](https://github.com/moq/moq4) from 4.14.7 to 4.15.2.

#Changelog

*Sourced from [Moq's changelog](https://github.com/moq/moq4/blob/master/CHANGELOG.md).*

> ## 4.15.2 (2020-11-26)
>
> #### Changed
>
> * Upgraded `System.Threading.Tasks.Extensions` dependency to version 4.5.4 (@JeffAshton, [#1108](devlooped/moq#1108))
>
>
> ## 4.15.1 (2020-11-10)
>
> #### Added
>
> * New method overloads for `It.Is`, `It.IsIn`, and `It.IsNotIn` that compare values using a custom `IEqualityComparer<T>` (@weitzhandler, [#1064](devlooped/moq#1064))
> * New properties `ReturnValue` and `Exception` on `IInvocation` to query recorded invocations return values or exceptions (@MaStr11, [#921](devlooped/moq#921), [#1077](devlooped/moq#1077))
> * Support for "nested" type matchers, i.e. type matchers that appear as part of a composite type (such as `It.IsAnyType[]` or `Func<It.IsAnyType, bool>`). Argument match expressions like `It.IsAny<Func<It.IsAnyType, bool>>()` should now work as expected, whereas they previously didn't. In this particular example, you should no longer need a workaround like `(Func<It.IsAnyType, bool>)It.IsAny<object>()` as originally suggested in [#918](devlooped/moq#918). (@stakx, [#1092](devlooped/moq#1092))
>
> #### Changed
>
> * Event accessor calls (`+=` and `-=`) now get consistently recorded in `Mock.Invocations`. This previously wasn't the case for backwards compatibility with `VerifyNoOtherCalls` (which got implemented before it was possible to check them using `Verify{Add,Remove}`). You now need to explicitly verify expected calls to event accessors prior to `VerifyNoOtherCalls`. Verification of `+=` and `-=` now works regardless of whether or not you set those up (which makes it consistent with how verification usually works). (@80O, @stakx, [#1058](devlooped/moq#1058), [#1084](devlooped/moq#1084))
> * Portable PDB (debugging symbols) are now embedded in the main library instead of being published as a separate NuGet symbols package (`.snupkg) (@kzu, [#1098](devlooped/moq#1098))
>
> #### Fixed
>
> * `SetupProperty` fails if property getter and setter are not both defined in mocked type (@stakx, [#1017](devlooped/moq#1017))
> * Expression tree argument not matched when it contains a captured variable &ndash; evaluate all captures to their current values when comparing two expression trees (@QTom01, [#1054](devlooped/moq#1054))
> * Failure when parameterized `Mock.Of<>` is used in query comprehension `from` clause (@stakx, [#982](devlooped/moq#982))
>
>
> ## 4.15.0
>
> This version was accidentally published as 4.15.1 due to an intermittent problem with NuGet publishing.

#Commits

- [`f2aa090`](devlooped/moq@f2aa090) ...
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

Successfully merging a pull request may close this issue.

2 participants