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

BeEquivalentTo incorrectly compare JsonElements? #2374

Open
Smurfa opened this issue Oct 10, 2023 · 4 comments
Open

BeEquivalentTo incorrectly compare JsonElements? #2374

Smurfa opened this issue Oct 10, 2023 · 4 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation feature

Comments

@Smurfa
Copy link

Smurfa commented Oct 10, 2023

Description

I am trying to compare if two json strings are equivalent (don't care about whitespaces or order). Using suggestion from previous issue: #1212 yields incorrect results.

Reproduction Steps

[Fact]
public void Test()
{
    var foo = JsonDocument.Parse("""{"test":"something"}""").RootElement;
    var bar = JsonDocument.Parse("""{"test":"something else"}""").RootElement;

    foo.Should().BeEquivalentTo(bar, opt => opt.ComparingByMembers<JsonElement>());
}

Expected behavior

Would expect this to fail, "something" != "something else".

Actual behavior

Test passes.

Regression?

No response

Known Workarounds

No response

Configuration

.NET 7.0
xunit 2.5.0
FluentAssertions 6.12.0

Other information

No response

Are you willing to help with a pull-request?

No

@jnyrup
Copy link
Member

jnyrup commented Oct 11, 2023

My snippet in #1212 only compares the ValueKind property of a JsonElement.
The design of JsonElement doesn't let us use the built-in structural equivalency which reflects over the properties on the type.
To make it work, you'll have to write a custom IEquivalencyStep.

Here's some code I hacked together in five minutes.
It works for your example, but it most likely doesn't work in the general case, so no guarantees are given.

[Fact]
public void Test()
{
    var foo = JsonDocument.Parse("""{"test":"something"}""").RootElement;
    var bar = JsonDocument.Parse("""{"test":"something else"}""").RootElement;

    foo.Should().BeEquivalentTo(bar, opt => opt.Using(new JsonObjectEquivalencyStep()));
}

public class JsonObjectEquivalencyStep : IEquivalencyStep
{
    public EquivalencyResult Handle(Comparands comparands, IEquivalencyValidationContext context, IEquivalencyValidator nestedValidator)
    {
        if (comparands.Subject is not JsonElement subject || comparands.Expectation is not JsonElement expectation)
        {
            return EquivalencyResult.ContinueWithNext;
        }

        if (subject.ValueKind is JsonValueKind.Object && expectation.ValueKind is JsonValueKind.Object)
        {
            var newComparands = new Comparands(subject.EnumerateObject(), expectation.EnumerateObject(), typeof(IEnumerable<JsonProperty>));
            nestedValidator.RecursivelyAssertEquality(newComparands, context);
            return EquivalencyResult.AssertionCompleted;
        }

        if (subject.ValueKind is JsonValueKind.Array && expectation.ValueKind is JsonValueKind.Array)
        {
            var newComparands = new Comparands(subject.EnumerateArray(), expectation.EnumerateArray(), typeof(IEnumerable<JsonElement>));
            nestedValidator.RecursivelyAssertEquality(newComparands, context);
            return EquivalencyResult.AssertionCompleted;
        }

        return EquivalencyResult.ContinueWithNext;
    }
}

Also the failure message is also not as pretty as it could be, as it uses some internal index over the json property name

Expected root[0] to be "test":"something else", but found "test":"something".

@dennisdoomen
Copy link
Member

This should also be covered by #2205

@Smurfa
Copy link
Author

Smurfa commented Oct 26, 2023

Sorry for late response and thanks for your replies @jnyrup and @dennisdoomen. Wanted to make sure if I was missing something obvious to make it work out of the box. But then I know it would require some custom comparison implementation to get it working. Cheers!

@dennisdoomen dennisdoomen added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Feb 14, 2024
@dennisdoomen
Copy link
Member

@jnyrup what do you think of the PR? It could be a first, rather independent step to proper support for System.Text.Json?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation feature
Projects
None yet
Development

No branches or pull requests

3 participants