constraints from ConstraintExpression/ConstraintBuilder are not reusable

Bug #532488 reported by blaz
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
NUnit Framework
High
Charlie Poole
NUnit V2
Fix Released
High
Charlie Poole

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()
        {
            Constraint expression = new NotConstraint(Is.Null);
            Assert.That(this, expression, "one");
            Assert.That(this, expression, "two");
        }
    }
}

Related branches

Revision history for this message
Olof Bjarnason (objarni) wrote :

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

Revision history for this message
blaz (blaz-lorger) wrote :

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.

Revision history for this message
Olof Bjarnason (objarni) wrote :

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

Revision history for this message
Charlie Poole (charlie.poole) wrote : RE: [Nunit-core] [Bug 532488] Re: constraintsfrom ConstraintExpression/ConstraintBuilder are not reusable
Download full text (3.6 KiB)

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()
> {
> ...

Read more...

Revision history for this message
Charlie Poole (charlie.poole) wrote :

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.

Changed in nunitv2:
status: New → Triaged
importance: Undecided → High
assignee: nobody → Charlie Poole (charlie.poole)
milestone: none → 2.5.6
Changed in nunitv2:
status: Triaged → In Progress
Revision history for this message
Charlie Poole (charlie.poole) wrote :

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

Changed in nunitv2:
status: In Progress → Fix Committed
Changed in nunitv2:
status: Fix Committed → Fix Released
Changed in nunit-3.0:
status: New → Triaged
importance: Undecided → High
tags: added: v2port
Changed in nunit-3.0:
assignee: nobody → Charlie Poole (charlie.poole)
status: Triaged → Fix Committed
milestone: none → 2.9.5
Changed in nunit-3.0:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers