Assert.Equal for JValue with mismatching types asserts to true.

[gurustron]

Not sure that this is worth fixing or not, and maybe it is actually will work in the next release (as I can see there is some refactoring going on) but consider the following (from SO question, some explanation for 2.4.2 xunit version there):

Repro:

[Fact]
public void Test2()
{
    var jvLeft = JToken.FromObject(1);
    var jvRight = JToken.FromObject("bar");

    // some "debug" checks
    Assert.True(jvLeft is JValue);
    Assert.Empty(jvLeft);
    Assert.Throws<FormatException>(() => (jvRight as IComparable).CompareTo(jvLeft));

    Assert.Equal(jvRight, jvLeft); // output: pass
}

Expected:
Fail

Actual:
Passes

Workaround
Use Assert.StrictEqual (works correctly for JValues) or more generic approach - Assert.True(JToken.DeepEquals(jvRight, jvLeft));.

[bradwilson]

This is because JToken derives from IEnumerable, so it gets treated like a collection. You can see this by flipping Assert.Equal to Assert.NotEqual:

Assert.NotEqual() Failure: Collections are equal
Expected: Not []
Actual:       []

The actual fix here would be for JSON.NET to implement one of the many equality/comparability interfaces provided in .NET. We currently look for and support:

• IEquatable<T> (1)
• IStructuralEquatable
• IComparable<T> (1)
• IComparable

(1) We support both the T of expected and actual when they're not the same type

We are not planning to add special-cased support for JSON.NET, as we don't special case anything that isn't part of the core framework itself. Third party libraries are expected to either conform to the comparison systems provided by .NET, or the testers are expected to use their own custom logic when the third party libraries aren't written to comply, as is the case with JSON.NET.

[gurustron]

@bradwilson please check out the linked SO question. JValue implements IComparable, the problem here is that it throws and the error is swallowed (maybe a more correct behavior would be to return false in this case?), so the JValue is compared as IEnumerable, but it is an empty IEnumerable from the library point of view.

We are not planning to add special-cased support for JSON.NET, as we don't special case anything that isn't part of the core framework itself.

Totally agree with this.

[bradwilson]

Hmm, I think if I changed the behavior, it would be to propagate the exception. Assuming "exception" means "not equal" isn't correct, IMO, because what if you were using Assert.NotEqual? Any exception would cause the code to unnecessarily pass.

I want to review this since I'm already catching it, and I assume I had a good reason for it. 😄

[bradwilson]

The commit that changed this is more than 6 years old: xunit/assert.xunit@6349e0c

This is the associated PR: xunit/assert.xunit#13

No reasoning there. Maybe @hughbe will remember? $10 says it was added for JSON.NET. 😂

[bradwilson]

Still poking around here, and I've determined that with the 2.5.0 assertion library, IComparable never comes into play because IEnumerable always takes precedence:

https://github.com/xunit/assert.xunit/blob/b4e17c53b3c1eb97d69146f275387611563bd395/EqualityAsserts.cs#L137-L146

So even removing the try/catch would have no impact any more.

[gurustron]

@bradwilson thank you!

Assuming "exception" means "not equal" isn't correct, IMO, because what if you were using Assert.NotEqual?

Yes, I agree that this is also not the clearest/cleanest solution (though an argument can be made here - if two things can't be compared they are definitely not equal 😊).