New It.Is overload for matching values using custom IEqualityComparer

[weitzhandler]

I'm looking for a way to easily compare multiple fields in an incoming class, to a given class.

Current code:

/* arrange */
var storeEntity = new Entity {  Id = "foo", Name = "bar" };
// alternatively
var storeEntity = new {  Id = "foo", Name = "bar" }; //anonymous class

/* act */
entityService.UpdateEntity("foo");

/* assert */
entityRepository
  .Verify(service => 
      service.Update(It.Is<Entity>(entity => entity.Id == storeEntity.Id && entity.Name = "bar" && ... ),
    Times.Once);

Desired code:

entityRepository
  .Verify(service => service.Update(It.IsEquivalentTo(storeEntity)),
    Times.Once);

I encounter this request over and over and it's compelling to me. I'm willing to contribute to this.

[weitzhandler]

BTW, what's the reason Match's constructor is internal? I understand there is Create, but that prevents us from creating inheritable matchers.

[weitzhandler]

Here's what I'm using at the moment:

public static class ItExtensions
{
  public static TValue ItIsEquivalentTo<TValue>(TValue expectedValue, IEqualityComparer<TValue> equalityComparer) =>
    Match.Create<TValue>(value => equalityComparer.Equals(expectedValue, value));

  public static TValue ItIsEquivalentTo<TValue>(TValue expectedValue) =>
    ItIsEquivalentTo(expectedValue, new PropertiesEqualityComparer<TValue>());

  private class PropertiesEqualityComparer<TValue> : EqualityComparer<TValue>
  {
    static readonly Lazy<PropertyInfo[]> _Properties = new Lazy<PropertyInfo[]>(() => typeof(TValue).GetProperties());
    static IEnumerable<PropertyInfo> Properties => _Properties.Value;
    static IEnumerable<object?> GetValues(TValue value) => Properties.Select(prop => prop.GetValue(value));
    static IEnumerable<(object? Value1, object? Value2)> GetValues(TValue value1, TValue value2) => 
      Properties.Select(prop => (prop.GetValue(value1), prop.GetValue(value2)));

    public override bool Equals([AllowNull] TValue x, [AllowNull] TValue y)
    {
      if (ReferenceEquals(x, y))
        return true;

      if (x == null)
        return y == null;
      else if (y == null)
        return false;

      return
        GetValues(x, y)
        .All(values => object.Equals(values.Value1, values.Value2));
    }

    public override int GetHashCode([DisallowNull] TValue obj)
    {
      if (obj == null)
        return 0;

      return GetValues(obj)
        .Select(val => val?.GetHashCode() ?? 0)
        .Aggregate((hash1, hash2) => hash1 ^ hash2);
    }
  }
}

Usage:

using static MyProject.Api.Tests.Unit.TestExtensions.ItExtensions;

... 

_PerTenantUserStoreMock
  .Verify(perTenantUserStore => 
      perTenantUserStore.CreateAsync(ItIsEquivalentTo(perTenantUser), defaultCancellationToken),
    Times.Once);

[stakx]

@weitzhandler your ItExtensions.ItIsEquivalentTo looks about right. Your example shows that most of the work actually lies in implementing an IEqualityComparer; defining the matcher is rather trivial.

On the one hand, your helper is generic enough that it very much looks like a feasible addition to the Moq library.

On the other hand, I'm a little bit afraid of the effect it will have on new users, due to the name for the new matcher:

Ideally, the matcher should end up being named It.IsEqualTo (not It.IsEquivalentTo) because it can deal with any kind of equality; "equivalence" actually stems from your particular comparer.

However, once we have a matcher called It.IsEqualTo, it'll be even easier for new users to (erroneously) assume that this is how you match precise values, when in reality you can just use constant values instead of a matcher. (This fear is not completely unfounded, every now and then we see people doing things like It.Is(x => x == 42) instead of simply 42.)

If it weren't for that, I'd be very much in favor of your proposed addition. For the reason just given, I'm not 100% sure whether it should be in the main library.

[weitzhandler]

Hey @stakx and thanks for you dedication and diligence!

Ideally, the matcher should end up being named It.IsEqualTo (not It.IsEquivalentTo) because it can deal with any kind of
equality; "equivalence" actually stems from your particular comparer.

I agree. I actually borrowed the name from FluentAssertions😋, but it works differently there, it compares the entire graph, which I think might be too much for this scope/sprint.

So can I PR this addition with some tests?
If yes, which repo? Any other instructions to keep in mind?

[stakx]

I would merge a PR only if I felt that my concerns regarding the matcher's name (see above) were reasonably addressed, or shown to be unfounded. That hasn't happened yet, so perhaps it's better if you waited a little longer (it would be a shame to have you spend time on a PR that then doesn't get merged). Let's see if more opinions/arguments are added to this discussion.

Regarding that hypothetical PR, are we in agreement that it would only encompass the matcher method (It.IsEqualTo) but not any specific IEqualityComparer implementation? (IMO the latter definitely don't belong in the Moq library, as they are more widely useful/applicable than just in conjunction with Moq.)

[weitzhandler]

I would merge a PR only if I felt that my concerns regarding the matcher's name

I don't have a strong opinion about the naming at all. Actually I'd simply add another overload to the existing Is, that takes an object and an IEqualityComparer<T>, plus another one that takes just an object and redirects to the other one specifying EqualityComparer<T>.Default - I guess accoring to what you said above, we don't want the second as to not mislead users of just going with a constant like, but in the other hand, sometimes you do want to specify a constant of a basetype or even an interface to soften the type-matching - but then again it would probably be useless without a comparer, so I'll just work on option one: a constant + a comparer.

are we in agreement that it would only encompass the matcher method

I agree. And in any case we can continue the discussion of whether we provide an implementation to that after the basic PR is implemented.

[weitzhandler]

Please have a look at this commit: moq/moq.spikes@fe9ccf0, and let me know if I'm headed the right direction.

BTW, I thought I should implement this in moq5, but then I see that the It methods have been marked as hidden. I guess I'm missing some basic understanding about what's the recommended way to go. Please advise.

[stakx]

You'll need to decide whether you want to work on Moq v4 (this repository) or Moq v5 (a whole different beast, and residing in the repository where you committed).

Like I already said above, I don't think spending time on a PR makes much sense unless we know we actually want to do this, so please understand that I'm not going to review at this time just yet.

[weitzhandler]

Like I already said above, I don't think spending time on a PR makes much sense unless we know we actually want to do this, so please understand that I'm not going to review at this time just yet.

Let's decide on an API then if at all then.

How about just one addition:

It.Is<T>(T value, IEqualityComparer<T> comparer);

How does that sound to you? Having only one overload that takes a comparer, would prevent users getting confused about using an inline constant.

Also consider add this overload that takes a comparer to the other ones (IsInRange, IsNotIn).

[weitzhandler]

You'll need to decide whether you want to work on Moq v4 (this repository) or Moq v5 (a whole different beast, and residing in the repository where you committed).

But I understand it's designed differently in v5 (it was hidden in intellisense and placed under "legacy"). What's the new recommended way of using this instead of It? Is this documented anywhere?

[stakx]

It.Is<T>(T value, IEqualityComparer<T> comparer);

Yes, this looks reasonable. While it does give It.Is two distinct meanings (which is somewhat bad), it prevents addition of a misleading It.IsEqualTo (which is good).

What's the new recommended way of using this instead of It?

I believe in Moq 5 you'd do Arg.Any<T>() or Arg.Any<T>(condition) (see there). Moq 5's API is quite different from Moq 4's and not directly comparable, thanks however for checking!

[weitzhandler]

Yes, this looks reasonable.

OK great, glad we're on the same page then. What about the rest of the functions, like It.IsInRange,IsIn/IsNotIn etc., do we want to have an overload that takes a comparer too?

Moq 5's API is quite different from Moq 4's

Well for now lemme just stick to 4 then. Should it turn out to be that awesome, and you guys might consider porting it into 5 lol...

[stakx]

What about the rest of the functions, like It.IsInRange, IsIn/IsNotIn etc., do we want to have an overload that takes a comparer too?

Good thought. That would perhaps be a good idea for It.IsIn and It.IsNotIn (add new method overloads, don't change the existing ones). Let's skip It.IsInRange for now, as it probably targets numeric types where a dedicated equality comparer isn't needed in most cases.

[weitzhandler]

don't change the existing ones

Thanks for the warning. My intention was actually to add a new one and port the existing one to it using EqualityComparer<T>.Default (given it automatically passes all tests).

[stakx]

My intention was actually to add a new one and port the existing one to it using EqualityComparer<T>.Default (given it automatically passes all tests).

That should be fine. I was mostly concerned about the method's signatures, not their implementation (as long as it still behaves in exactly the same way).

[weitzhandler]

That should be fine. I was mostly concerned about the method's signatures

Nah, won't be breaking any code 😱