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

Assert.Equivalent and System.Text.Json #2696

Closed
jasonj333 opened this issue Mar 16, 2023 · 4 comments
Closed

Assert.Equivalent and System.Text.Json #2696

jasonj333 opened this issue Mar 16, 2023 · 4 comments

Comments

@jasonj333
Copy link

Using .NET 7, we recently switched from JSON.Net to System.Text.Json and a few of our tests failed. Previously, we were using Assert.Equivalent to compare 2 JSON objects, now we are getting an exception within xUnit:

System.ArgumentException
An item with the same key has already been added. Key: Item
   at System.Collections.Generic.Dictionary`2.TryInsert(TKey key, TValue value, InsertionBehavior behavior)
   at System.Collections.Generic.Dictionary`2.Add(TKey key, TValue value)
   at System.Linq.Enumerable.ToDictionary[TSource,TKey,TElement](IEnumerable`1 source, Func`2 keySelector, Func`2 elementSelector, IEqualityComparer`1 comparer)
   at System.Linq.Enumerable.ToDictionary[TSource,TKey,TElement](IEnumerable`1 source, Func`2 keySelector, Func`2 elementSelector)
   at Xunit.Internal.AssertHelper.<>c.<GetGettersForType>b__1_0(Type _type) in /_/src/xunit.assert/Asserts/Sdk/AssertHelper.cs:line 53
   at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd(TKey key, Func`2 valueFactory)
   at Xunit.Internal.AssertHelper.GetGettersForType(Type type) in /_/src/xunit.assert/Asserts/Sdk/AssertHelper.cs:line 31
   at Xunit.Internal.AssertHelper.VerifyEquivalenceReference(Object expected, Object actual, Boolean strict, String prefix, HashSet`1 expectedRefs, HashSet`1 actualRefs) in /_/src/xunit.assert/Asserts/Sdk/AssertHelper.cs:line 230
   at Xunit.Internal.AssertHelper.VerifyEquivalence(Object expected, Object actual, Boolean strict, String prefix, HashSet`1 expectedRefs, HashSet`1 actualRefs) in /_/src/xunit.assert/Asserts/Sdk/AssertHelper.cs:line 165
   at Xunit.Internal.AssertHelper.VerifyEquivalence(Object expected, Object actual, Boolean strict, String prefix, HashSet`1 expectedRefs, HashSet`1 actualRefs) in /_/src/xunit.assert/Asserts/Sdk/AssertHelper.cs:line 144
   at Xunit.Internal.AssertHelper.VerifyEquivalenceEnumerable(IEnumerable expected, IEnumerable actual, Boolean strict, String prefix, HashSet`1 expectedRefs, HashSet`1 actualRefs) in /_/src/xunit.assert/Asserts/Sdk/AssertHelper.cs:line 200
   at Xunit.Internal.AssertHelper.VerifyEquivalence(Object expected, Object actual, Boolean strict, String prefix, HashSet`1 expectedRefs, HashSet`1 actualRefs) in /_/src/xunit.assert/Asserts/Sdk/AssertHelper.cs:line 163
   at Xunit.Internal.AssertHelper.VerifyEquivalence(Object expected, Object actual, Boolean strict) in /_/src/xunit.assert/Asserts/Sdk/AssertHelper.cs:line 71

Simple repo:

        [Fact]
        public void XunitBug()
        {
            var o1 = new JsonObject();
            var o2 = new JsonObject();

            o1.Add("field1", JsonValue.Create("1"));
            o2.Add("field1", JsonValue.Create("1"));

            o1.Add("field2", JsonValue.Create("2"));
            o2.Add("field2", JsonValue.Create("2"));

            Assert.Equal(o1.ToJsonString(), o2.ToJsonString()); //passes
            Assert.Equivalent(o1, o2, true); //fails
        }

We are using Visual Studio 17.4.4 and xUnit 2.4.2

@bradwilson
Copy link
Member

bradwilson commented Mar 17, 2023

The problem is three-fold:

  1. We forgot to exclude indexers from the property list, which is causing the ArgumentException you're experiencing. This is trivial to fix, since we cannot use indexers when doing equivalence tests (we'd have no idea what indexer values to pass).

  2. We only check for IComparable with value types. JsonValue<T> is not a value type.

  3. Even if we fixed issue 2, JsonValue<T> does not implement IComparable (or any of the traditional .NET equivalence checks to compare itself to another JsonValue<T>), so even after fixing issue 1 and 2, so end up treating it like a traditional reference type. The public properties that are here include Parent, which of course will fail since these two nodes have different parents in the tree.

JsonValue<T> implements no interfaces at all, and does not even override Equals(). It is clear to me that JsonValue<T> was never designed for any generalized equality validation in mind, which makes it impossible to use in a generalized equality (or equivalent) assertion. I am reluctant to add specific support for JsonValue<T>, not the least of which reason because we couldn't take a dependency on it, so our support for it would be based on runtime reflection, for which all equivalence tests would pay a price.

Unless and until JsonValue<T> is fixed to include the ability to compare itself to another instance of JsonValue<T>, this is likely never going to work as-is. Your only recourse at this moment is to use some other assert for validation.

You should be wary about using .ToJsonString() as your example above does, since JsonObject preserves the order that properties were added. Changing your test to this fails:

[Fact]
public void XunitBug()
{
    var o1 = new JsonObject();
    var o2 = new JsonObject();

    o1.Add("field1", JsonValue.Create("1"));
    o1.Add("field2", JsonValue.Create("2"));

    o2.Add("field2", JsonValue.Create("2"));
    o2.Add("field1", JsonValue.Create("1"));

    Assert.Equal(o1.ToJsonString(), o2.ToJsonString());
}
Assert.Equal() Failure
                 ↓ (pos 7)
Expected: {"field1":"1","field2":"2"}
Actual:   {"field2":"2","field1":"1"}
                 ↑ (pos 7)

The best generalized solution I can think of is to implement (or find) an IEqualityComparer<JsonObject>, and pass an instance of it to Assert.Equal(o1, o2, comparer);. A fully generalized solution will be non-trivial, as there are a lot of corner cases to consider, but a specialized one may suit your needs.

@jasonj333
Copy link
Author

Thanks, I didn't want to use strings for the very reason you mentioned. I was just making sure they were equal in this simple case. FluentAssertions has ComparingByMembers that looks like it may work or maybe I just implement a simple non-recursive comparer which should meet our needs. Thanks again.

@bradwilson
Copy link
Member

The problem with a generalized "comparing by members" is that that's exactly what we do. 😁 JsonValue<T> isn't a value, it's a value + hierarchy tracking (where you are in the tree), and it's the hierarchy tracking that makes it fail. I'd be shocked if this works in a fluent assertion system unless they've already written some custom code around System.Text.Json specifically.

@lateapexearlyspeed
Copy link

Hi @jasonj333 there is Xunit assertion json extension package which can assert json data as "json way". For your example, just write similar as:

string actualJson = """
    {
      "field1": "1",
      "field2": "2"
    }
    """;

string expectedJson = """
    {
      "field2": "2",
      "field1": "1"
    }
    """;

JsonAssertion.Equivalent(expectedJson, actualJson);

maybe I just implement a simple non-recursive comparer which should meet our needs

the library considered all recursive cases for json.
Hope can try it, any feedback is appreciated, thanks : https://github.com/lateapexearlyspeed/Lateapexearlyspeed.JsonSchema

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants