Ignore order of properties when comparing snapshots

[vajsm]

Is your feature request related to a problem? Please describe.

Consider the scenario where tests are failing due to the exact same object being serialized differently (properties in a different order) on subsequent test runs, or when snapshots were created on one machine, and used in tests on another one.

The order of properties when serializing objects is not guaranteed by the framework (unless you're using [JsonProperty(Order = 1)]), and it caused me quite a headache when some tests in my project were failing when they were run in a different order. I don't have access to that codebase anymore, so I won't give a working (well, failing) example, but I was able to narrow it down to the fact that Snapshooter deserializes the string snapshots to JTokens inside of JsonSnapshotComparer.CompareSnapshots, and then serializes them back again to strings. This is the place where the order of properties gets changed, and as a result, the tests are failing because the strings aren't exactly the same.

If the objects were indeed identical, I believe the tests should pass. The way I see it, Snapshooter shouldn't rely on the actual order of JSON properties in the snapshot files by default, or at least should provide a possibility to configure such behavior.

Describe the solution you'd like

Snapshooter could order properties alphabetically when serializing/deserializing objects.

Describe alternatives you've considered

Additionally, there could be something like IgnorePropertyOrder(bool shouldIgnore) setting added to MatchOptions class, so that the order could be taken into consideration in scenarios when the order of properties matters for some reason.

Additional context

Let me know how you see it, and maybe I can help with the implementation - if you feel like it's a feature that should be added.

[sarvasana]

Add this to your solution:

    public sealed class PropertiesSortedAlphabeticallySnapshotSerializerSettings : SnapshotSerializerSettings
    {
        /// <inheritdoc />
        public override bool Active => true;

        /// <inheritdoc />
        public override int Order => int.MaxValue;

        /// <inheritdoc />
        public override JsonSerializerSettings Extend(JsonSerializerSettings settings)
        {
            settings.ContractResolver = PropertiesSortedAlphabeticallyContractResolver.Instance;
            return settings;
        }

        private class PropertiesSortedAlphabeticallyContractResolver : DefaultContractResolver
        {
            static PropertiesSortedAlphabeticallyContractResolver() =>
                Instance = new PropertiesSortedAlphabeticallyContractResolver();

            public static PropertiesSortedAlphabeticallyContractResolver Instance { get; }

            protected override IList<JsonProperty> CreateProperties(
                Type type, MemberSerialization memberSerialization)
            {
                IList<JsonProperty> properties = base.CreateProperties(type, memberSerialization);

                properties = properties.OrderBy(p => p.PropertyName).ToList();

                return properties;
            }
        }
    }

[sarvasana]

It does make me wonder why the default contract resolver, the ChildFirstContractResolver, is not consistently sorting members.

        private class ChildFirstContractResolver : DefaultContractResolver
        {
            static ChildFirstContractResolver() { Instance = new ChildFirstContractResolver(); }

            public static ChildFirstContractResolver Instance { get; }

            protected override IList<JsonProperty> CreateProperties(
                Type type, MemberSerialization memberSerialization)
            {
                IList<JsonProperty> properties = base.CreateProperties(type, memberSerialization);

                if (properties != null)
                {
                    properties = properties.OrderBy(p =>
                    {
                        IEnumerable<Type> d = ((Type)p.DeclaringType).BaseTypesAndSelf().ToList();
                        return 1000 - d.Count();
                    }).ToList();
                }

                return properties;
            }
        }

[sarvasana]

Could be thread-safety issue in JsonPropertyCollection.

https://github.com/JamesNK/Newtonsoft.Json/blob/57025815e564d36821acf778e2c00d02225aab35/Src/Newtonsoft.Json/Serialization/JsonPropertyCollection.cs#L74

Used in:
https://github.com/JamesNK/Newtonsoft.Json/blob/master/Src/Newtonsoft.Json/Serialization/DefaultContractResolver.cs#L1392

@JamesNK

[vajsm]

Alright, you've made me think about this again @sarvasana.

I do not know why the order of properties wasn't consistent either. A repro would be useful, but I'm in no position to provide one :) Your guess is as good as mine. Thread-safety issue idea seems interesting, would you mind to elaborate? Not sure if I see this.

Newtonsoft aside, I can imagine another reason why it could come in handy to be able to ignore the order of properties. And for this one, I can give repro steps.

Consider the following: you have a class with some properties in it, and you already have snapshots.

public class Class
{
    public string A { get; set; } = "Hello";
    public string B { get; set; } = "World";
}

// And a little test:

[Fact]
public void Test() 
{
    Class obj = new();
    Snapshot.Match(obj);
}

Now, you decide to change the order of these properties in the class.

public class Class
{
    public string B { get; set; } = "World";
    public string A { get; set; } = "Hello";
}

For no reason at all. You just want to watch the world burn, and now your unit tests fail, indicating an object mismatch. The objects are identical in content, but the snapshots are not. If you only care about content, this is a false negative.

I agree this would be rather rare and if someone is particularly worried about this, they could use your ChildFirstContractResolver. But I feel like comparison that cares about order of properties could be opt-in rather than default. Here's what my original idea was about: d2f637f - with JToken.DeepEquals instead of string comparison, the tests will only fail if the properties change in name or value, or in number, but not if their order changes. That could be configurable.

[nrjohnstone]

The PropertiesSortedAlphabeticallySnapshotSerializerSettings works great but since the configuration of the SnapshotSerializerSettings is magic reflection based there is very little ability to introduce this into an existing Snapshooter project with hundreds of snapshots without spending a very long time fixing things...

I added a small static AsyncLocal "Enabled" public property to allow each test fixture to slowly opt in for alphabetical ordering which allows a gradual introduction of it across various test fixtures.

[jeremy8883]

Thanks for the PropertiesSortedAlphabeticallySnapshotSerializerSettings solution. But where in the solution should this be actually set up? I can't find anything in the docs.

I found SnapshotSerializerSettings.DefaultJsonSerializerSettings, but I don't know how to modify it

[sarvasana]

Thanks for the PropertiesSortedAlphabeticallySnapshotSerializerSettings solution. But where in the solution should this be actually set up? I can't find anything in the docs.

I found SnapshotSerializerSettings.DefaultJsonSerializerSettings, but I don't know how to modify it

I currently don't use this project, but is adding it to your solution not enough?

[jeremy8883]

Oh right, yes sorry. I had just assumed it needed to be invoked somewhere. Thank you!