Add support for KeyValuePair<,> in AssertEqualityComparer to enable collections in values

[nachtjasmin]

While working on a .NET 6 library, I've stumbled across the following bug:

When asserting a class that's deriving from System.Net.Http.Headers.HttpHeaders, the equality assertion does not work as intended.

Test case

using Xunit;
using System.Net.Http.Headers;

namespace xunit_http_headers_collection_repro;

public class MyHttpHeaders : HttpHeaders {}

public class UnitTest1
{
    [Fact]
    public void Test()
    {
        var a = new MyHttpHeaders();
        var b = new MyHttpHeaders();

        a.Add("test", "test");
        b.Add("test", "test");

        Assert.Equal(a, b);
    }
}

The output message

  Failed xunit_http_headers_collection_repro.UnitTest1.Test [18 ms]
  Error Message:
   Assert.Equal() Failure: Collections differ
           ↓ (pos 0)
Expected: [["test"] = ["test"]]
Actual:   [["test"] = ["test"]]
           ↑ (pos 0)

Clearly the collections are identical, so I assume a bug in here. The xunit version is 2.5.3 and there's a full repro repository here.

[bradwilson]

HttpHeaders does not implement any of the equatable interfaces that we look for (IComparable, IEquatable, IStructuralEquatable, etc.), so we fall back to default logic.

HttpHeaders does implement IEnumerable<> so our default logic treats it like a collection. The values is enumerates are of type KeyValuePair<string, IEnumerable<string>>, which also does not implement any of the equatable interfaces. So as we iterate through each item, we end up using default Equals logic to determine if the values are equals.

Since KeyValuePair is a struct, that means the default behavior is to compare all of its members using Equals (which are Key and Value). The key type is string so comparisons are fine, but the value type is IEnumerable<string>, which will end up comparing via reference. Since the two collections stored internally are different instances (regardless of whether those instances contain identical values), that will fail.

We have not created any special logic for HttpHeaders, nor do I think we should. In general we expect the things that people want to compare for equality either fall into the bucket of things we have special treatment for, or they are responsible for their own definition of equality. HttpHeaders definitely falls into the latter category.

We also have not created any special logic for handling KeyValuePair<K,V>. This one, I'm not 100% sure about whether we should. We obviously already have special logic for dictionaries (and if HttpHeaders had presented itself as a dictionary, rather than a simple collection, we would have chosen that path).

Let me think on whether we want to add special case code for KeyValuePair.

[bradwilson]

Yeah, I think this is a good idea, and it's pretty simple to add.

[bradwilson]

Available in v2 2.5.4-pre.6
Available in v3 0.1.1-pre.300

[nachtjasmin]

Damn, thanks for the very detailed explanation and the quick fix! I appreciate it a lot! 😮