views:

54

answers:

3

I have two custom classes, ChangeRequest and ChangeRequests, where a ChangeRequests can contain many ChangeRequest instances.

public class ChangeRequests : IXmlSerializable, ICloneable, IEnumerable<ChangeRequest>,
    IEquatable<ChangeRequests> { ... }

public class ChangeRequest : ICloneable, IXmlSerializable, IEquatable<ChangeRequest>
    { ... }

I am trying to do a union of two ChangeRequests instances. However, duplicates do not seem to be removed. My MSTest unit test is as follows:

var cr1 = new ChangeRequest { CRID = "12" };
var crs1 = new ChangeRequests { cr1 };
var crs2 = new ChangeRequests
               {
                   cr1.Clone(),
                   new ChangeRequest { CRID = "34" }
               };
Assert.AreEqual(crs1[0], crs2[0], "First CR in both ChangeRequests should be equal");
var unionedCRs = new ChangeRequests(crs1.Union<ChangeRequest>(crs2));
ChangeRequests expected = crs2.Clone();
Assert.AreEqual(expected, unionedCRs, "Duplicates should be removed from a Union");

The test fails in the last line, and unionedCRs contains two copies of cr1. When I tried to debug and step through each line, I had a breakpoint in ChangeRequest.Equals(object) on the first line, as well as in the first line of ChangeRequest.Equals(ChangeRequest), but neither were hit. Why does the union contain duplicate ChangeRequest instances?

Edit: as requested, here is ChangeRequests.Equals(ChangeRequests):

public bool Equals(ChangeRequests other)
{
    if (ReferenceEquals(this, other))
    {
        return true;
    }

    return null != other && this.SequenceEqual<ChangeRequest>(other);
}

And here's ChangeRequests.Equals(object):

public override bool Equals(object obj)
{
    return Equals(obj as ChangeRequests);
}

Edit: I overrode GetHashCode on both ChangeRequest and ChangeRequests but still in my test, if I do IEnumerable<ChangeRequest> unionedCRsIEnum = crs1.Union<ChangeRequest>(crs2);, unionedCRsIEnum ends up with two copies of the ChangeRequest with CRID 12.

Edit: something has to be up with my Equals or GetHashCode implementations somewhere, since Assert.AreEqual(expected, unionedCRs.Distinct(), "Distinct should remove duplicates"); fails, and the string representations of expected and unionedCRs.Distinct() show that unionedCRs.Distinct() definitely has two copies of CR 12.

+2  A: 

I don't believe that Assert.AreEqual() examines the contents of the sequence - it compares the sequence objects themselves, which are clearly not equal.

What you want is a SequenceEqual() method, that will actually examine the contents of two sequences. This answer may help you. It's a response to a similar question, that describes how to compare to IEnumerable<> sequences.

You could easily take the responder's answer, and create an extension method to make the calls look more like assertions:

public static class AssertionExt
{
  public static bool AreSequencesEqual<T>( IEnumerable<T> expected, 
                                           IEnumerable<T> sequence )
  {
    Assert.AreEqual(expected.Count(), sequence .Count()); 

    IEnumerator<Token> e1 = expected.GetEnumerator(); 
    IEnumerator<Token> e2 = sequence .GetEnumerator(); 

    while (e1.MoveNext() && e2.MoveNext()) 
    { 
        Assert.AreEqual(e1.Current, e2.Current); 
    }
  }
}

Alternatively you could use SequenceEqual(), to compare the sequences, realizing that it won't provide any information about which elements are not equal.

LBushkin
Trying `SequenceEqual` still failed. I have overridden `Equals` on `ChangeRequests`, so calling `AreEqual` on two `ChangeRequests` should work. My `ChangeRequests.Equals` override compares the sequences.
Sarah Vessels
@Sarah - it's odd that `SequenceEqual` wouldn't work - it should use the default equality comparer, which in turn, should test T for being `IEquatable<T>`, and use the equality comparer appropriate to that implementation.
LBushkin
A: 

As LBushkin says, Assert.AreEqual will just call Equals on the sequences.

You can use the SequenceEqual extension method though:

Assert.IsTrue(expected.SequenceEqual(unionedCRs));

That won't give much information if it fails, however.

You may want to use the test code we wrote for MoreLINQ which was sequence-focused - if the sequences aren't equal, it will specify in what way they differ. (I'm trying to get a link to the source file in question, but my network connection is rubbish.)

Jon Skeet
I have overridden `Equals` on `ChangeRequests`, so using `AreEqual` should work, I think. Replacing the last line of my test with `Assert.IsTrue(expected.SequenceEqual<ChangeRequest>(unionedCRs), "Duplicates should be removed from a Union");` still failed.
Sarah Vessels
@Sarah: With Assert.Equal, it's not calling Equals on your ChangeRequests type - it's calling Equals on the *sequence*, which won't override Equals. It should work with SequenceEqual though.
Jon Skeet
+1  A: 

Make sure your GetHashCode implementation is consistent with your Equals - the Enumerable.Union method does appear to use both.

You should get a warning from the compiler if you've implemented one but not the other; it's still up to you to make sure that both methods agree with each other. Here's a convenient summary of the rules: http://stackoverflow.com/questions/371328/why-is-it-important-to-override-gethashcode-when-equals-method-is-overriden-in-c

Tim Robinson