Assert.Equivalent stack overflows when comparing FileInfo objects

[Blokyk]

Version: 2.5.0

The following test crashes with a stack overflow, provided that the file exist:

[Fact]
public void SameFileInfosAreEquivalent() {
    var path = "testhost.dll"; // or any other file (not folder) that exists
    Assert.Equivalent(new FileInfo(path), new FileInfo(path));
}

Log dump:

The active test run was aborted. Reason: Test host process crashed : Stack overflow.
Repeat 21051 times:
--------------------------------
   at Xunit.Internal.AssertHelper.VerifyEquivalenceReference(System.Object, System.Object, Boolean, System.String, System.Collections.Generic.HashSet`1<System.Object>, System.Collections.Generic.HashSet`1<System.Object>)
   at Xunit.Internal.AssertHelper.VerifyEquivalence(System.Object, System.Object, Boolean, System.String, System.Collections.Generic.HashSet`1<System.Object>, System.Collections.Generic.HashSet`1<System.Object>)
--------------------------------
   at Xunit.Internal.AssertHelper.VerifyEquivalence(System.Object, System.Object, Boolean)
   at Xunit.Assert.Equivalent(System.Object, System.Object, Boolean)
   at Recline.Tests.Options.Tests.SameFileInfoAreEquivalent()
   at [test host stuff]

[Blokyk]

After a bit a debugging, it seems like this is because FileInfo.Directory has a the following property:

public DirectoryInfo Root => new DirectoryInfo(Path.GetPathRoot(FullPath)!);

And since DirectoryInfo has reference equality, .Root and .Root.Root are different, the actualRefs/expectedRefs tables can't see that it's just a circular reference 🙃

Is this even fixable? Or would this require changing the property's definition in dotnet/runtime ?

[bradwilson]

So a quick though on using Assert.Equal vs. Assert.Equivalent here.

I added Assert.Equivalent to test that two objects were structurally equal without being forced to be the same type. The goal was to make certain types of testing easier: for example, deserializing an object (like from JSON or XML) and being able to easily test it (partially or fully) by describing that object with something like an anonymous type.

Here's an example asserting a complex value using just anonymous types to describe the expected structure:

xunit/src/xunit.v3.assert.tests/Asserts/EquivalenceAssertsTests.cs

Lines 488 to 495 in 966a5b7

	[Fact]
	public void Success()
	{
	var expected = new { Shallow = new { Value1 = 42, Value2 = "Hello, world!" }, Value3 = 21.12m };
	var actual = new DeepClass { Value3 = 21.12m, Shallow = new ShallowClass { Value1 = 42, Value2 = "Hello, world!" } };
	Assert.Equivalent(expected, actual);
	}

Your example looks more like you're after equality than equivalence. (More about the problems with that in a second, because I'm guessing you already tried Assert.Equal and had a failure.)

One way to continue to use Assert.Equivalent is to describe the "shape" of the FileInfo you expect to get back (with only the properties you care about testing), just like my example above:

[Fact]
public void Xunit2767()
{
    var expected = new { Name = "README.md", DirectoryName = @"C:\Dev\xunit\xunit.v2", Exists = true };
    var actual = new FileInfo(@"C:\Dev\xunit\xunit.v2\README.md");

    Assert.Equivalent(expected, actual);
}

The difference here is that we use the expected value to decide what to look at in the actual value. When the expected value is a FileInfo, we're going to explore all public properties, and that causes the stack overflow; when the expected value is the anonymous object, then the only properties we inspect will be the ones you put into the expectation (in this case, we only care about the values of .Name, .DirectoryName, and .Exists, and we ignore any additional properties on the actual object).

Is this even fixable?

The answer is that I think there are two separate issues here, and both of them are fixable:

• To be able to just pass two FileInfo objects into Assert.Equivalent and have it pass, the answer is that we have to update Assert.Equivalent to special-case FileInfo. We have a special case already in the code for DateTime and DateTimeOffset because of a circular reference problem: https://github.com/xunit/assert.xunit/blob/d0a234a523e8a898dd7f0d36d97876a68be8b6a9/Sdk/AssertHelper.cs#L237-L240

  We could add a similar helper to compare FileInfo and/or DirectoryInfo to prevent the stack overflow problem by just ignoring Directory and only comparing the local properties, much like the example above.

• We should probably also add a "depth cap" when running Assert.Equivalent which would flag the error before it gets to the point of being a full blown StackOverflowExecption. It would generally raise itself with a message that says something like "we hit the maximum depth while comparing these two objects, which may indicate that you have a circular reference problem".

would this require changing the property's definition in dotnet/runtime ?

Neither of my proposals above require any changes to .NET itself. Any attempts to fix it there would likely incur extra expense (in terms of computation and/or memory usage) that the team would probably be reluctant to accept, especially since it's really only solving a testing problem, not a runtime problem.

That said, I do think it would be worth opening a PR to add IEquatable<> implementations to both FileInfo and DirectoryInfo. That would allow them to be used directly by Assert.Equal (which is what I think should be used to compare two concrete objects of the same type), and allow them to pass without forcing the developer to write their own custom comparison function (or equality comparer object).

[bradwilson]

BTW, here's what the Assert.Equal with a custom comparison function might look like that does the same thing as Assert.Equivalent with the anonymous type:

[Fact]
public void Xunit2767()
{
    var expected = new FileInfo(@"C:\Dev\xunit\xunit.v2\README.md");
    var actual = new FileInfo(@"C:\Dev\xunit\xunit.v2\README.md");

    Assert.Equal(
        expected, actual,
        (e, a) => e.Name == a.Name && e.DirectoryName == a.DirectoryName && e.Exists == a.Exists
    );
}

[bradwilson]

Shipping in v2: 2.5.1-pre.25
Shipping in v3: 0.1.1-pre.274

[Blokyk]

Sorry for the late reply -- but thank you a lot for the fix!

Just to answer this:

One way to continue to use Assert.Equivalent is to describe the "shape" of the FileInfo you expect to get back

In this case I was a few levels deep into an anonymous object and re-using an existing static readonly value that had a FileInfo property (mostly cause I'm lazy lol)