VerifyTests / Verify

Verify is a snapshot testing tool that simplifies the assertion of complex data models and documents.
MIT License
2.52k stars 142 forks source link

BUG: Can't verify objects with a CancellationToken who's CancellationTokenSource has been Disposed. #1134

Closed LorneCash closed 7 months ago

LorneCash commented 9 months ago

This Unit test runs fine:

    [TestMethod]
    public async Task Test()
    {
        var cts = new CancellationTokenSource();
        cts.Cancel();
        var test = new TaskCanceledException("test", null, cts.Token);
        //cts.Dispose();

        await Verify(test);
    }

And I get this in the .received.txt:

{
  Type: TaskCanceledException,
  CancellationToken: {
    IsCancellationRequested: true,
    CanBeCanceled: true,
    WaitHandle: {
      SafeWaitHandle: {
        IsInvalid: false,
        IsClosed: false
      }
    }
  },
  Message: test
}

But if I try to run the same test after disposing the cancellation token like this:

    [TestMethod]
    public async Task Test()
    {
        var cts = new CancellationTokenSource();
        cts.Cancel();
        var test = new TaskCanceledException("test", null, cts.Token);
        cts.Dispose();

        await Verify(test);
    }

Verify throws this Exception: Argon.JsonSerializationException: Error getting value from 'WaitHandle' on 'System.Threading.CancellationToken'. ---> System.ObjectDisposedException: The CancellationTokenSource has been disposed.

Stack Trace: 
    CancellationTokenSource.get_WaitHandle()
    GetWaitHandle(Object )
    DynamicValueProvider.GetValue(Object target) line 70
    --- End of inner exception stack trace ---
    DynamicValueProvider.GetValue(Object target) line 74
    CustomValueProvider.GetValue(Object target) line 22
    JsonSerializerInternalWriter.CalculatePropertyValues(JsonWriter writer, Object value, JsonContainerContract contract, JsonProperty member, JsonProperty property, JsonContract& memberContract, Object& memberValue) line 424
    JsonSerializerInternalWriter.SerializeObject(JsonWriter writer, Object value, JsonObjectContract contract, JsonProperty member, JsonContainerContract collectionContract, JsonProperty containerProperty) line 390
    JsonSerializerInternalWriter.SerializeValue(JsonWriter writer, Object value, JsonContract valueContract, JsonProperty member, JsonContainerContract containerContract, JsonProperty containerProperty) line 121
    JsonSerializerInternalWriter.SerializeObject(JsonWriter writer, Object value, JsonObjectContract contract, JsonProperty member, JsonContainerContract collectionContract, JsonProperty containerProperty) line 396
    JsonSerializerInternalWriter.SerializeValue(JsonWriter writer, Object value, JsonContract valueContract, JsonProperty member, JsonContainerContract containerContract, JsonProperty containerProperty) line 121
    JsonSerializerInternalWriter.Serialize(JsonWriter jsonWriter, Object value, Type type) line 30
    JsonSerializer.SerializeInternal(JsonWriter jsonWriter, Object value, Type type) line 619
    Converter.Write(VerifyJsonWriter writer, InfoBuilder value) line 50
    WriteOnlyJsonConverter`1.Write(VerifyJsonWriter writer, Object value) line 7
    WriteOnlyJsonConverter.WriteJson(JsonWriter writer, Object value, JsonSerializer serializer) line 12
    JsonSerializerInternalWriter.SerializeConvertible(JsonWriter writer, JsonConverter converter, Object value, JsonContract contract, JsonContainerContract collectionContract, JsonProperty containerProperty) line 543
    JsonSerializerInternalWriter.SerializeValue(JsonWriter writer, Object value, JsonContract valueContract, JsonProperty member, JsonContainerContract containerContract, JsonProperty containerProperty) line 114
    JsonSerializerInternalWriter.Serialize(JsonWriter jsonWriter, Object value, Type type) line 30
    JsonSerializer.SerializeInternal(JsonWriter jsonWriter, Object value, Type type) line 619
    JsonSerializer.Serialize(JsonWriter jsonWriter, Object value) line 585
    JsonFormatter.AsJson(VerifySettings settings, Counter counter, Object value) line 7
    InnerVerifier.TryGetRootTarget(Object root, Nullable`1& target) line 133
    InnerVerifier.VerifyInner(Object root, Func`1 cleanup, IEnumerable`1 targets, Boolean doExpressionConversion) line 11
    InnerVerifier.Verify(Object target) line 71
    <<Verify>b__0>d.MoveNext() line 97

Obviously I'm not actually trying to Verify this simple example, or I would just not dispose the CancellationTokenSource. However, since disposing them is recommended it's not surprising that this situation would come up sooner or later.

I'd love to see the output something like this:

{
  Type: TaskCanceledException,
  CancellationToken: {
    IsCancellationRequested: true,
    CanBeCanceled: true,
    WaitHandle: <Disposed>
  },
  Message: test
}

But omitting the WaitHandle property would me more than acceptable also.

If there's an easy workaround that I've missed, please forgive my ignorance, and help me out by pointing me in the right direction.

SimonCropp commented 9 months ago

will this work for you

[Fact]
public async Task DisposedTaskCanceledException()
{
    var cancel = new CancelSource();
    cancel.Cancel();
    var test = new TaskCanceledException("test", null, cancel.Token);
    cancel.Dispose();

    await Verify(test)
        .IgnoreMembersThatThrow<ObjectDisposedException>();
}

or globally

[ModuleInitializer]
public static void Init() =>
    VerifierSettings
        .IgnoreMembersThatThrow<ObjectDisposedException>();
LorneCash commented 9 months ago

Amazing, .IgnoreMembersThatThrow() works perfectly.

I can't believe I missed that. Thank you so much!

SimonCropp commented 9 months ago

here is the doco https://github.com/VerifyTests/Verify/blob/main/docs/serializer-settings.md#members-that-throw

I can't believe I missed that.

please dont hesitate to ask questions or raise issues for (what u think are) missing apis. it is a large surface area, and i am happy to point to the relevant docs.

and i am on twitter @SimonCropp and mastodon simoncropp@hachyderm.io if u want a more adhoc conversation

LorneCash commented 9 months ago

I feel like it would be slightly better if it said something like: WaitHandle: <Disposed>

Or: WaitHandle: <ObjectDisposedException>

instead of: WaitHandle: null

But I'm not complaining!

I'm also slightly curious if you'd want to look into fixing this specific case so that it would not require using .IgnoreMembersThatThrow<T>() due to the fact that it throws from the .Verify() method, but that's a design decision for you and really makes no difference to me.

SimonCropp commented 9 months ago

yeah those are good suggestions. i will look into them

LorneCash commented 8 months ago

I created a classes derived from Argon.DefaultContractResolver and Argon.IValueProvider.

using System.Reflection;
using Argon;

public class VerifyReceivedContractResolver : DefaultContractResolver
{
    /// <inheritdoc />
    protected override JsonProperty CreateProperty(MemberInfo member, MemberSerialization memberSerialization)
    {
        var property = base.CreateProperty(member, memberSerialization);
        if (property is { Ignored: false, ValueProvider: { } valueProvider })
        {
            property.ValueProvider = new SafeValueProvider(valueProvider);
        }
        return property;
    }

    private class SafeValueProvider : IValueProvider
    {
        private readonly IValueProvider UnderlyingValueProvider;

        public SafeValueProvider(IValueProvider underlyingValueProvider) => UnderlyingValueProvider = underlyingValueProvider;

        public void SetValue(object target, object? value) => UnderlyingValueProvider.SetValue(target, value);

        public object? GetValue(object target)
        {
            try
            {
                return UnderlyingValueProvider.GetValue(target);
            }
            catch (Exception ex)
            {
                var message = $"[{ex.GetType().FullName}]{ex.Message}";
                while (ex.InnerException is { } exInner)
                {
                    message += $" ==> [{exInner.GetType().FullName}]{exInner.Message}";
                    ex = exInner;
                }
                return message;
            }
        }
    }
}

This works really well for me because it's not specific to CancellationToken, WaitHandle, or any specific Exception type at all. It (the serialization) just works and then I can decide to ignore or scurb as I please.

Here's what the output looks like:

CancellationToken: {                                                                      
  IsCancellationRequested: true,                                                          
  CanBeCanceled: true,                                                                    
  WaitHandle: [Argon.JsonSerializationException]Error getting value from 'WaitHandle' on 'System.Threading.CancellationToken'. ==> [System.ObjectDisposedException]The CancellationTokenSource has been disposed.
},                                                                                        
LorneCash commented 7 months ago

@SimonCropp With 23.5.1 (your latest update), it seems that the CanBeCanceled property of TaskCanceledException is just called Token, not sure if that was intentional but it confused me for a sec.

I'm also noticing that when the token is disposed WaitHandle is missing. I'm sure that one was intentional. Personally I prefer it my way with the error message but your way makes sense too. 👍

And lastly I now understand that my VerifyReceivedContractResolver was really a fix for Argon rather than Verify so I get why you might not want to do that.

Thanks again for all your work on this amazing project!

SimonCropp commented 7 months ago

can u try 23.5.2

LorneCash commented 7 months ago

Beautiful! 💯