The `Assert.AreEqual(object, object)` is very error prone and poor design

[whoisj]

Description

Due the fact that the Assert static class has an AreEqual(object, object) the compiler is always able to find a valid match during compilation if a type changes on one side of the assertion. This means all failures must be found very late in the game (read: after compilation and tests have been run).

This is generally not desirable. Either the AreEqual(object, object) method should go away, or a more strict version of AreEqual should introduced. An ideal API might be

void Equality<T>(T expected, T actual) where T: IEquatable; 
void Equality<T>(T expected, T actual, IEqualityComparer<T> comparer);
void SameObject(object expected, object actual); // performs ReferenceEquals(expected, actual)

With this triple alone, a test suite would cover 99% of equality tests without having to worry about the duck-typing mess we have today.

Steps to reproduce

Setup a test with Assert.AreEqual(0, obj.value) with obj.value starting as an int, then change the type to TimeSpan (or whatever). It'll compile happily but fail when tests are run.

In a tiny project this is not a major issue, but in a large project where compilation take 45+ minutes and test runs can last an hour or more, this is a major time waster.

Expected behavior

.NET solutions not using duck-typing.

Actual behavior

MSTest uses duck-typing because ... ?

Environment

Windows 10, Visual Studio 2017, NetFx 4.6.2, stock MSTest nuget packages.

[pvlakshm]

@whoisj, This looks like a good usecase for writing a custom assertion. Would you like to give that a try? Writing custom assertions is documented in this RFC. There is also a sample available here: Sample,

[whoisj]

@pvlakshm yeah sure, the unforunate part is a custom assertion is private and not shared as part of the core library; meaning other adopters of the library do not benefit. Additionally, the flawed (by C# 4.5+ standards) design remains and is too easy for a consumer to adopt - which raises the cost of accepting contributions because now I have another thing to check for in every code review.

Honestly, this library is getting old. Have a "mode shift" (whereby old features were disabled and newer ones enabled) by configuration would be a huge benefit.

[AbhitejJohn]

@whoisj: Would the below API from here work?

AreEqual<T>(T expected, T actual)

You could mandate the use of this as opposed to the more general AreEqual(object, object). Agreed that this could be changed but pulling it out would be a breaking change.

[whoisj]

@abhitejJohn how does the comparison in that method work?

Lacking the where T: IComparable<T> or the third parameter IEqualityComparer<T> comparer makes me slightly concerned. In generaly, yes it is better then AreEqual(object, object) but it'll default back to (object, object) if !(T is IComparable<T>), no?

Breaking changes aren't that concerning when they happen at the ABI signature layer and at major milestones because consumers get build failures during compilation stages, It's the failures that come after build + test (or worse after release to customers) which cause heartburn.

[AbhitejJohn]

Yeah, that's odd these APIs don't accept an IComparable<T>(we might need to revisit the default set of assertion APIs - @pvlakshm, @harshjain2). We eventually just do an object.Equals(). I'm not sure what you mean by defaulting back but as long as the API called is AreEqual<T>(T,T) if any of the parameters passed in is not T then it would be a build break. Hope that answers the query.

Yup, we can consider deprecating this in the next major milestone.

[whoisj]

@abhitejJohn while I agree the AreEqual<T>(T,T) solves the duck typing problem, it does not necessarily solve the equality problem. C# Object.Equals(Object) method only tests reference equality. It is identical to ReferenceEquals(Object, Object), or in C (void *)pt31 == (void *)prt2;

Please keep me in the loop as this is a major headache for me. I've been asked to use the MSTest framework in preference to NUint/XUnit and this is has been causing test escapes and unexpected failures with every set of tests I've converted.

Thanks! :smile:

[julealgon]

Assert.AreEqual<T> is not good enough due to type inference as well: if you make the type explicit on the call, most tools will (correctly) point out that you are being redundant and suggest removing the type.

Having the object version is terrible design after generics was introduced and should definitely be removed, or we'll never have proper compile-time safety.

I've also run though the same problem OP describes multiple times so having this corrected would be incredibly useful.

[lostmsu]

When is this going to be addressed? In many cases an overload with IEqualityComparer<T> is required.

[AbhitejJohn]

Thanks folks. Would you mind giving the issue a 👍 and we'll prioritize this. Sounds like we'd want to mark the API as obsolete for a few release so users are aware and remove it in the next major. /cc: @nohwnd

[Evangelink]

We will add some analyzers that will prevent some issues on this. In parallel, we will make the object, object overload obsolete.

[Evangelink]

As our next version is a breaking change, I feel like it's better to include this change directly into it.

To sum it up, we will:

• Change AreSame/AreNotSame overloads to accept generic instead of object, this way we ensure we cannot compare 2 different things. (https://github.com/microsoft/testfx/pull/1430)
• Remove AreEqual/AreNotEqual overloads with "object, object" to leave only the overloads with generic type. Note that object.Equals(obj1, obj2) is correctly calling overrides of Equals.
• Add overloads with the IEqualityComparer<T>

[eXpl0it3r]

If type A implements IEquatable<A> and type B also implements IEquatable<A> (yes of A), then you could call Assert.AreEqual(instanceOfA, instanceOfB) before and it would work, now T is expected to be either both A or both B, even though they could be "equatabled".

I mean it's questionable to compare two different types, but I haven't really seen this scenario discussed as breaking, so I thought I mention it here. Guess, I'll have to convert on the test, rather than inside the Equals functions.

class A : IEquatable<A>
{
    public bool Equals(A other) { ... }
}

class B : IEquatable<A>
{
    public bool Equals(A other) { ... }
}

// ...

[TestMethod]
public void Test()
{
    var instanceOfA = new A();
    var instanceOfB = new B();

    Assert.AreEqual(instanceOfA, instanceOfB);
}

[Evangelink]

@eXpl0it3r Thanks for the report, I definitely didn't consider this case. I guess we could simply add a new overload as public static void AreEqual<T>(IEquatable<T>? expected, IEquatable<T>? actual) and it would fix this case without breaking some other scenario (from what I can think of).