constraints from ConstraintExpression/ConstraintBuilder are not reusable

[blaz]

Constraint that I receive from syntax helper (Is.Not.Null) is not reusable, but I guess this is more general problem.
I found this problem when upgrading nunit.framework.dll from v2.4.6 to v.2.5.3. In both cases tests were executed witn NUnit 2.5.3 console.
Worst thing about this problem seems that tests could actually pass when they should fail if you reuse constraint.

Here is simple example that demonstrates the problem:

using NUnit.Framework;
using NUnit.Framework.Constraints;

namespace NUnit_2_5_3_bug
{
    [TestFixture]
    public class FailToReuseConstraint
    {
        /// <summary>
        /// Constraint from <see cref="ConstraintExpression"/>.
        /// </summary>
        /// <remarks>
        /// Demonstrates that constraint received from syntax helper is not reusable.
        /// </remarks>
        [Test]
        public void FromSyntaxHelper()
        {
            Constraint expression = Is.Not.Null;
            Assert.That(this, expression, "one");
            Assert.That(this, expression, "two"); // this step fails on NUnit.2.5.3
        }

        /// <summary>
        /// How NUnit 2.4 did it (i think).
        /// </summary>
        [Test]
        public void NotConstraint()
        {
            Constraint expression = new NotConstraint(Is.Null);
            Assert.That(this, expression, "one");
            Assert.That(this, expression, "two");
        }
    }
}

[Olof Bjarnason (objarni)]

Are constraints supposed to be "reused"? I have used NUnit for years and have never found that useful..

[blaz (blaz-lorger)]

In some rare cases reuse could be useful.
Main problem is that I was unable to find any documentation specifying that constraints should not be reused, or that constrains supplied by NUnit are not reusable. Actually in most cases under most conditions constraints are reusable.

I think that this bug could be fixed either by making constrains reusable or by placing appropriate warning in documentation (http://www.nunit.org/index.php?p=constraintModel&r=2.5.3), or constrains that are not reusable should fail on reuse.

[Olof Bjarnason (objarni)]

Good suggestions, any of them would be fine with me ..

[Charlie Poole (charlie.poole)]

It's a bad idea to have constraints fail when they are used improperly.

If you do that then...

Constraint c = new NonReusableConstraint(...);
Assert.That( actual, c ); // This fails
Assert.That( actual, new NotConstrant( c ) ); // This succeeds!

In an earlier version of NUnit we had this problem when using
a constraint that expects a collection with a non-collection.
The constraint was failing but its negation would succeed.
Now we throw an ArgumentException in this sort of situation.

In the case of a non-reusable constraint we could throw an
exception, but only if the constraint "knew" it was being
reused.

Charlie

> -----Original Message-----
> From:
> <email address hidden>
> [mailto:<email address hidden>
> et] On Behalf Of blaz
> Sent: Friday, March 05, 2010 3:16 AM
> To: <email address hidden>
> Subject: [Nunit-core] [Bug 532488] Re: constraintsfrom
> ConstraintExpression/ConstraintBuilder are not reusable
>
> In some rare cases reuse could be useful.
> Main problem is that I was unable to find any documentation
> specifying that constraints should not be reused, or that
> constrains supplied by NUnit are not reusable. Actually in
> most cases under most conditions constraints are reusable.
>
> I think that this bug could be fixed either by making
> constrains reusable or by placing appropriate warning in
> documentation
> (http://www.nunit.org/index.php?p=constraintModel&r=2.5.3),
> or constrains that are not reusable should fail on reuse.
>
> --
> constraints from ConstraintExpression/ConstraintBuilder are
> not reusable
> https://bugs.launchpad.net/bugs/532488
> You received this bug notification because you are a member
> of NUnit Developers, which is subscribed to NUnit V2.
>
> Status in NUnit V2 Test Framework: New
>
> Bug description:
> Constraint that I receive from syntax helper (Is.Not.Null) is
> not reusable, but I guess this is more general problem.
> I found this problem when upgrading nunit.framework.dll from
> v2.4.6 to v.2.5.3. In both cases tests were executed witn
> NUnit 2.5.3 console.
> Worst thing about this problem seems that tests could
> actually pass when they should fail if you reuse constraint.
>
> Here is simple example that demonstrates the problem:
>
> using NUnit.Framework;
> using NUnit.Framework.Constraints;
>
> namespace NUnit_2_5_3_bug
> {
> [TestFixture]
> public class FailToReuseConstraint
> {
> /// <summary>
> /// Constraint from <see cref="ConstraintExpression"/>.
> /// </summary>
> /// <remarks>
> /// Demonstrates that constraint received from syntax
> helper is not reusable.
> /// </remarks>
> [Test]
> public void FromSyntaxHelper()
> {
> Constraint expression = Is.Not.Null;
> Assert.That(this, expression, "one");
> Assert.That(this, expression, "two"); // this
> step fails on NUnit.2.5.3
> }
>
> /// <summary>
> /// How NUnit 2.4 did it (i think).
> /// </summary>
> [Test]
> public void NotConstraint()
> {
> ...

[Charlie Poole (charlie.poole)]

After discussion on the mailing list and in the context of another bug, Dale King pointed out that the source of the problem is that a constraint expression is resolved by Assert.That on it's first use and a pointer to the resolved constraint is what is actually used. What we should do for a reusable constraint is to pre-resolve it and then save that pointer.

Note that the problem does not arise with simple constraints like Is.Null. It's only when the constraint has pending operators to be resolved - like Is.Not.Null - that we see the issue.

Requiring the user to resolve the constraint first seems like too much. The Resolve() method is an explicit interface implementation and so requires a cast for it even to be called. I'll experiment with a wrapper class that allows reuse and add some info about reuse to the docs.

[Charlie Poole (charlie.poole)]

The current fix makes use of a new class, ReusableConstraint. Still waiting for comments on this solution so it may change.