Hi Charlie, I agree with you except when you mention that we use == for
convenience. I am wondering if rather than convenience it is instead an
unintentionally incorrect way to check for nullness. I am not sure if
switching from using equality operators in favor of ReferenceEquals would
break existing supported scenarios in addition to opening up the one
proposed by Meg, I'll try to look a bit more into it and see if I can
somehow come up with a solution which gives us some confidence that it will
not break existing code. As it stands now, allowing equality between
objects which look like they're null is a valid scenario and sounds like a
natural extension to allowing objects to override their Equals method,
unless it turns out to be technically unfeasible.

Simone


On Sat, Mar 23, 2013 at 6:25 AM, Charlie Poole <email address hidden> wrote:

> Hi Simone,
>
>
> On Fri, Mar 22, 2013 at 2:59 AM, Simone Busoli
> <email address hidden>wrote:
>
> > On Fri, Mar 22, 2013 at 6:39 AM, Charlie Poole <email address hidden>
> > wrote:
> >
> > > The NUnit equality model has handled null reference the same way for 13
> > > years, so I hesitate to change it in any way that will break existing
> > > applications.
> > >
> >
> > I don't think that the changes this would require would necessarily break
> > existing applications. Did you assume that because you know they would?
> As
> > I said, the changes I did in the branch I linked earlier don't break any
> > NUnit tests, although I did not spend much time trying to figure out
> other
> > side-effects.
> >
>
> No, I don't assume they would. I was only trying to set a criterion for any
> changes.
>
>
> > > I agree with Simone that IsNull should not pass with anything except
> the
> > > null reference.
> > >
> > > If there is a way we can make the two AreEqual tests pass without
> > breaking
> > > other tests, then I might consider that.
> >
> >
> > As I said, all NUnit tests except the one provided by Meg which uses the
> > NullConstraint pass.
> >
>
> How did you do this without bypassing the null test for other objects?
>
>
> > > However, I'm dubious of the value
> > > of such an addition since use of AreEqual is not the way to test that
> the
> > > NullObject in question works. The right way is to call the object's
> > Equals
> > > overload directly and find out what it returns.
> > >
> >
> > I am also doubtful about the usefulness, I never myself had such a
> > requirement for a null object to compare to null, but I think it's
> > generally just an extension to the normal equality comparison. In our
> > EqualConstraint we are returning false as soon as either actual or
> expected
> > == null, but the operator can be overridden, so it would probably be more
> > correct to use ReferenceEquals(actual|expeced, null) in that case and try
> > to use object.Equals if !(actual|expected).ReferenceEquals(null)
> (although
> > (actual|expected) == null might be true).
> >
>
> Well, we use == for convenience, but the intent is to call ReferenceEquals.
> The general rule is that any two null references are equal and a null
> reference
> can't be equal to a non-null reference.
>
> > Testing by using Assert.AreEqual verifies that the object's
> implementation
> > > of equality matches NUnit's. I don't think that's what you want to
> test.
> > >
> >
> > I think that NUnit should fallback to the implementation of object.Equals
> > that actual and expected eventually override, which it is already doing,
> > except that it is not doing it if either actual or expected == null.
> >
>
> Well, of course you can't call object.Equals on a null reference, which is
> why the check is necessary in the first place. Since we default to the
> Equals implementation of the expected value, we could theoretically
> only apply the null logic to expected, but applying it symetrically seems
> to make the most sense.
>
>
> >
> > > I'm inclined to reject this on the grounds of utility, but I'd like to
> > hear
> > > the other side first.
> > >
> >
> > Sure, let's see if Meg has any other input.
> >
> >
> > >
> > > Charlie
> > >
> > >
> > > On Thu, Mar 21, 2013 at 3:26 PM, Simone Busoli
> > > <email address hidden>wrote:
> > >
> > > > Looks like a valid scenario in my opinion. It's not straightforward
> to
> > > > get right as we're doing all sorts of equality in NUnit but I put
> > > > together a spike which passes all tests plus the ones Meg posted here
> > > > except the Is.Null one. I think that one should not be supported as
> > > > Is.Null should check for reference equality, which you cannot
> override.
> > > >
> > > > I would like to hear what Charlie thinks too.
> > > >
> > > > lp:~simone.busoli/nunitv2/null-equality
> > > >
> > > > --
> > > > You received this bug notification because you are subscribed to
> NUnit
> > > > Extended Testing Platform.
> > > > https://bugs.launchpad.net/bugs/1157828
> > > >
> > > > Title:
> > > >   EqualityComparer does not respect value-equality for null
> > > >
> > > > To manage notifications about this bug go to:
> > > > https://bugs.launchpad.net/nunitv2/+bug/1157828/+subscriptions
> > > >
> > >
> > > --
> > > You received this bug notification because you are a member of NUnit
> > > Developers, which is subscribed to NUnit V2.
> > > https://bugs.launchpad.net/bugs/1157828
> > >
> > > Title:
> > >   EqualityComparer does not respect value-equality for null
> > >
> > > Status in NUnit V2 Test Framework:
> > >   New
> > >
> > > Bug description:
> > >   I am using a third-party library that includes Null Objects with
> value
> > >   comparisons that return true for null.  However, the NUnit equality
> > >   comparer uses reference equality (via the == operator with
> > >   System.Object references) to check for null and returns false if only
> > >   one input is the null pointer.  This means that the value-equality
> > >   comparison for the custom type is never reached.
> > >
> > >   The following is a quick & dirty implementation of a Null Object with
> > >   a few tests that demonstrate the problem:
> > >
> > >   public class MyObject
> > >   {
> > >       public static MyObject NullObject = new MyObject(-1);
> > >
> > >       public int Value { get; set; }
> > >
> > >       public MyObject(int value)
> > >       {
> > >           this.Value = value;
> > >       }
> > >
> > >       public static bool operator ==(MyObject left, MyObject right)
> > >       {
> > >           if (ReferenceEquals(left, NullObject) ||
> ReferenceEquals(left,
> > > null))
> > >               return (ReferenceEquals(right, NullObject) ||
> > > ReferenceEquals(right, null));
> > >
> > >           else if (ReferenceEquals(right, NullObject) ||
> > > ReferenceEquals(right, null))
> > >               return false;
> > >
> > >           return Equals(left, right);
> > >       }
> > >
> > >       public static bool operator !=(MyObject left, MyObject right)
> > >       {
> > >           return !(left == right);
> > >       }
> > >       public override bool Equals(object obj)
> > >       {
> > >           if (ReferenceEquals(obj, NullObject) || ReferenceEquals(obj,
> > > null))
> > >               return (ReferenceEquals(this, NullObject));
> > >
> > >           else if (ReferenceEquals(this, NullObject))
> > >               return false;
> > >
> > >           MyObject myobj = obj as MyObject;
> > >           if (myobj == null)
> > >               return false;
> > >
> > >           return this.Value == myobj.Value;
> > >       }
> > >
> > >       public override int GetHashCode()
> > >       {
> > >           return base.GetHashCode();
> > >       }
> > >
> > >   }
> > >
> > >   [TestFixture]
> > >   public class TestNullObject
> > >   {
> > >       // TEST PASSES
> > >       [Test]
> > >       public void TestIsTrue()
> > >       {
> > >           Assert.IsTrue(MyObject.NullObject == null);
> > >           Assert.IsTrue(null == MyObject.NullObject);
> > >       }
> > >
> > >       // TESTS FAIL
> > >       [Test]
> > >       public void TestIsNull()
> > >       {
> > >           Assert.IsNull(MyObject.NullObject);
> > >       }
> > >
> > >       [Test]
> > >       public void TestAreEqual1()
> > >       {
> > >           Assert.AreEqual(MyObject.NullObject, null);
> > >       }
> > >
> > >       [Test]
> > >       public void TestAreEqual2()
> > >       {
> > >           Assert.AreEqual(null, MyObject.NullObject);
> > >       }
> > >   }
> > >
> > > To manage notifications about this bug go to:
> > > https://bugs.launchpad.net/nunitv2/+bug/1157828/+subscriptions
> > >
> > > _______________________________________________
> > > Mailing list: https://launchpad.net/~nunit-core
> > > Post to     : <email address hidden>
> > > Unsubscribe : https://launchpad.net/~nunit-core
> > > More help   : https://help.launchpad.net/ListHelp
> > >
> >
> > --
> > You received this bug notification because you are subscribed to NUnit
> > Extended Testing Platform.
> > https://bugs.launchpad.net/bugs/1157828
> >
> > Title:
> >   EqualityComparer does not respect value-equality for null
> >
> > To manage notifications about this bug go to:
> > https://bugs.launchpad.net/nunitv2/+bug/1157828/+subscriptions
> >
>
> --
> You received this bug notification because you are a member of NUnit
> Developers, which is subscribed to NUnit V2.
> https://bugs.launchpad.net/bugs/1157828
>
> Title:
>   EqualityComparer does not respect value-equality for null
>
> Status in NUnit V2 Test Framework:
>   New
>
> Bug description:
>   I am using a third-party library that includes Null Objects with value
>   comparisons that return true for null.  However, the NUnit equality
>   comparer uses reference equality (via the == operator with
>   System.Object references) to check for null and returns false if only
>   one input is the null pointer.  This means that the value-equality
>   comparison for the custom type is never reached.
>
>   The following is a quick & dirty implementation of a Null Object with
>   a few tests that demonstrate the problem:
>
>   public class MyObject
>   {
>       public static MyObject NullObject = new MyObject(-1);
>
>       public int Value { get; set; }
>
>       public MyObject(int value)
>       {
>           this.Value = value;
>       }
>
>       public static bool operator ==(MyObject left, MyObject right)
>       {
>           if (ReferenceEquals(left, NullObject) || ReferenceEquals(left,
> null))
>               return (ReferenceEquals(right, NullObject) ||
> ReferenceEquals(right, null));
>
>           else if (ReferenceEquals(right, NullObject) ||
> ReferenceEquals(right, null))
>               return false;
>
>           return Equals(left, right);
>       }
>
>       public static bool operator !=(MyObject left, MyObject right)
>       {
>           return !(left == right);
>       }
>       public override bool Equals(object obj)
>       {
>           if (ReferenceEquals(obj, NullObject) || ReferenceEquals(obj,
> null))
>               return (ReferenceEquals(this, NullObject));
>
>           else if (ReferenceEquals(this, NullObject))
>               return false;
>
>           MyObject myobj = obj as MyObject;
>           if (myobj == null)
>               return false;
>
>           return this.Value == myobj.Value;
>       }
>
>       public override int GetHashCode()
>       {
>           return base.GetHashCode();
>       }
>
>   }
>
>   [TestFixture]
>   public class TestNullObject
>   {
>       // TEST PASSES
>       [Test]
>       public void TestIsTrue()
>       {
>           Assert.IsTrue(MyObject.NullObject == null);
>           Assert.IsTrue(null == MyObject.NullObject);
>       }
>
>       // TESTS FAIL
>       [Test]
>       public void TestIsNull()
>       {
>           Assert.IsNull(MyObject.NullObject);
>       }
>
>       [Test]
>       public void TestAreEqual1()
>       {
>           Assert.AreEqual(MyObject.NullObject, null);
>       }
>
>       [Test]
>       public void TestAreEqual2()
>       {
>           Assert.AreEqual(null, MyObject.NullObject);
>       }
>   }
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/nunitv2/+bug/1157828/+subscriptions
>
> _______________________________________________
> Mailing list: https://launchpad.net/~nunit-core
> Post to     : <email address hidden>
> Unsubscribe : https://launchpad.net/~nunit-core
> More help   : https://help.launchpad.net/ListHelp
>