New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

InRange-Constraint must work with object #1674

Closed
NUnitTester opened this Issue Jul 15, 2016 · 17 comments

Comments

Projects
None yet
5 participants
@NUnitTester

NUnitTester commented Jul 15, 2016

The following code does work even if myObj is from a class no special interface is implemented:

Assert.That(myObj, Is.EqualTo(expcted).Using(myComparer));
Assert.That(myObj, Is.InRange(expcted1, expcted2).Using(myComparer));

There must be a inheritance of IComparable, or it does not compile. (NUnit 3.4.1)
This is an error, because it must also work with objects.

  • the type of myObj and expected1 I cannot change, because it is a external type from a library
  • a comparer is supported, so it should work
  • the constructor of InRangeConstraint calls the Compare-Methode of IComparable of expected1 not regarding the comparer in the Using to check the relation between from and to. This is a big bug in my eyes, using the wrong comparer.
@CharliePoole

This comment has been minimized.

Show comment
Hide comment
@CharliePoole

CharliePoole Jul 15, 2016

Member

This has been the definition of RangeConstraint for many years. I take it you are saying that it should be sufficient for the two expected values to implement IComparable. Is that right?

Can you post a bit of code that shows what you would like to see working differently?

As possibly a second issue, I think you are saying that the comparer in Using is being ignored. Again, an example we can work from would help.

Member

CharliePoole commented Jul 15, 2016

This has been the definition of RangeConstraint for many years. I take it you are saying that it should be sufficient for the two expected values to implement IComparable. Is that right?

Can you post a bit of code that shows what you would like to see working differently?

As possibly a second issue, I think you are saying that the comparer in Using is being ignored. Again, an example we can work from would help.

@NUnitTester

This comment has been minimized.

Show comment
Hide comment
@NUnitTester

NUnitTester Jul 28, 2016

First issue: There is no possible way that I can add a IComparable to a class because it is from a library or not useful in productive case. So I wrote a Comparer for NUnit-Test only wanting to be used in Range-Asserts.

public class MyVersionClass /* : IComparable */ { ... }

MyVersionClass expectedObject = ...;

Assert.That(myObject, Is.InRange(expectedObject, expectedObject) .Using(myComparer));
Error, does not compile, but
Assert.That(myObject, Is.EqualTo(expectedObject) .Using(myComparer));
does!

NUnitTester commented Jul 28, 2016

First issue: There is no possible way that I can add a IComparable to a class because it is from a library or not useful in productive case. So I wrote a Comparer for NUnit-Test only wanting to be used in Range-Asserts.

public class MyVersionClass /* : IComparable */ { ... }

MyVersionClass expectedObject = ...;

Assert.That(myObject, Is.InRange(expectedObject, expectedObject) .Using(myComparer));
Error, does not compile, but
Assert.That(myObject, Is.EqualTo(expectedObject) .Using(myComparer));
does!

@NUnitTester

This comment has been minimized.

Show comment
Hide comment
@NUnitTester

NUnitTester Jul 28, 2016

Second issue: Look at the constructor of the RangeConstraint, then the error is obvious:

public RangeConstraint(IComparable from, IComparable to) : base((object) from, (object) to) { **if (this.comparer.Compare((object) from, (object) to) > 0) throw new ArgumentException("from must be less than to");** this.from = from; this.to = to; }
The Comparer ist called to check if from is not greater than to. It is not right because the comparer given by using does give other results. And of cause, see issue 1, the objects should not be comparable at all. It should also work only with a given comparer in using.

NUnitTester commented Jul 28, 2016

Second issue: Look at the constructor of the RangeConstraint, then the error is obvious:

public RangeConstraint(IComparable from, IComparable to) : base((object) from, (object) to) { **if (this.comparer.Compare((object) from, (object) to) > 0) throw new ArgumentException("from must be less than to");** this.from = from; this.to = to; }
The Comparer ist called to check if from is not greater than to. It is not right because the comparer given by using does give other results. And of cause, see issue 1, the objects should not be comparable at all. It should also work only with a given comparer in using.

@CharliePoole

This comment has been minimized.

Show comment
Hide comment
@CharliePoole

CharliePoole Jul 28, 2016

Member

@NUnitTester
First Issue: Does this work if you use Is.GreaterThanOrEqualTo(from).And.LessThanOrEqualTo(to)?

We should definitely handle these two expressions consistently.

Second issue:
So, we are using the default comparer before it's replaced. That's an error for sure.

Member

CharliePoole commented Jul 28, 2016

@NUnitTester
First Issue: Does this work if you use Is.GreaterThanOrEqualTo(from).And.LessThanOrEqualTo(to)?

We should definitely handle these two expressions consistently.

Second issue:
So, we are using the default comparer before it's replaced. That's an error for sure.

@NUnitTester

This comment has been minimized.

Show comment
Hide comment
@NUnitTester

NUnitTester Jul 29, 2016

Assert.That(myObject, Is.GreaterThanOrEqualTo(from).And.LessThanOrEqualTo(to).Using(myComparer));
myObject is < than from. Passes Assert because LessThanOrEqualTo uses not the given Comparer, only GreaterThanOrEqualTo.

The correct way to write and working seems to be:
Assert.That(myObject, Is.GreaterThanOrEqualTo(from).Using(myComparer)).And.LessThanOrEqualTo(to).Using(myComparer));

NUnitTester commented Jul 29, 2016

Assert.That(myObject, Is.GreaterThanOrEqualTo(from).And.LessThanOrEqualTo(to).Using(myComparer));
myObject is < than from. Passes Assert because LessThanOrEqualTo uses not the given Comparer, only GreaterThanOrEqualTo.

The correct way to write and working seems to be:
Assert.That(myObject, Is.GreaterThanOrEqualTo(from).Using(myComparer)).And.LessThanOrEqualTo(to).Using(myComparer));

@CharliePoole CharliePoole added pri:high and removed pri:normal labels Jul 29, 2016

@CharliePoole

This comment has been minimized.

Show comment
Hide comment
@CharliePoole

CharliePoole Jul 29, 2016

Member

Yes, Using applies to each comparison separately due to the left-to-right processing of the expression. That's one of the main reasons for having a range constraint.

I think this definitely shows the problem as a bug. If the expanded expression works, the range expression is expected to work.

Member

CharliePoole commented Jul 29, 2016

Yes, Using applies to each comparison separately due to the left-to-right processing of the expression. That's one of the main reasons for having a range constraint.

I think this definitely shows the problem as a bug. If the expanded expression works, the range expression is expected to work.

@JustinRChou

This comment has been minimized.

Show comment
Hide comment
@JustinRChou

JustinRChou Oct 19, 2016

Contributor

Gave up on the default comparer.

Would it be possible to check if the user did use Using for objects that did not implement IComparable?

Or default it to a IComparer comparing on the ToString()?

Contributor

JustinRChou commented Oct 19, 2016

Gave up on the default comparer.

Would it be possible to check if the user did use Using for objects that did not implement IComparable?

Or default it to a IComparer comparing on the ToString()?

JustinRChou added a commit to JustinRChou/nunit that referenced this issue Oct 19, 2016

nunit#1674 Using InRange with objects and no IComparable
Added constructor for objects in rangeconstraint

Updated Is and Not for new constructor.

Added unit test for case when class did not implement IComparable but
provides a IComparer through Using.
@NUnitTester

This comment has been minimized.

Show comment
Hide comment
@NUnitTester

NUnitTester Oct 21, 2016

If Using is used, always the comparer given by using must be used. Only if there is no using at all, IComparable is used.
It is also a common case to give an other comparer for nunit as the implementation does by implementing IComparable.

In my eyes, the test, if from is smaller or equal tomust be done later, not in constructor at all.

NUnitTester commented Oct 21, 2016

If Using is used, always the comparer given by using must be used. Only if there is no using at all, IComparable is used.
It is also a common case to give an other comparer for nunit as the implementation does by implementing IComparable.

In my eyes, the test, if from is smaller or equal tomust be done later, not in constructor at all.

JustinRChou added a commit to JustinRChou/nunit that referenced this issue Oct 30, 2016

nunit#1674 Removed !SILVERLIGHT and !NETCF defs
Updated to remove !SILVERLIGHT and !NETCF defs.

JustinRChou added a commit to JustinRChou/nunit that referenced this issue Oct 30, 2016

nunit#1674 Removed !NETCF and !SILVERLIGHT
Removed !NETCF and !SILVERLIGHT

@rprouse rprouse removed the hacktoberfest label Nov 9, 2016

@JustinRChou

This comment has been minimized.

Show comment
Hide comment
@JustinRChou

JustinRChou Jan 4, 2017

Contributor

I am going through this to see if my PR is missing anything.

So far InRange issue when passing objects without IComparer in the constructor is resolved.

And using
InRange(from, to).Using(myComparer)
isn't producing a different result from Is.GreaterThanOrEqualTo(from).Using(myComparer)).And.LessThanOrEqualTo(to).Using(myComparer)

Contributor

JustinRChou commented Jan 4, 2017

I am going through this to see if my PR is missing anything.

So far InRange issue when passing objects without IComparer in the constructor is resolved.

And using
InRange(from, to).Using(myComparer)
isn't producing a different result from Is.GreaterThanOrEqualTo(from).Using(myComparer)).And.LessThanOrEqualTo(to).Using(myComparer)

JustinRChou added a commit to JustinRChou/nunit that referenced this issue Jan 4, 2017

nunit#1674 Using InRange with objects and no IComparable
Added constructor for objects in rangeconstraint

Updated Is and Not for new constructor.

Added unit test for case when class did not implement IComparable but
provides a IComparer through Using.

JustinRChou added a commit to JustinRChou/nunit that referenced this issue Jan 4, 2017

nunit#1674 Removed !NETCF and !SILVERLIGHT
Removed !NETCF and !SILVERLIGHT

JustinRChou added a commit to JustinRChou/nunit that referenced this issue Jan 4, 2017

JustinRChou added a commit to JustinRChou/nunit that referenced this issue Jan 4, 2017

@NUnitTester

This comment has been minimized.

Show comment
Hide comment
@NUnitTester

NUnitTester Jan 9, 2017

Fine, I will look forward to the next release and see it in production.

NUnitTester commented Jan 9, 2017

Fine, I will look forward to the next release and see it in production.

@NUnitTester

This comment has been minimized.

Show comment
Hide comment
@NUnitTester

NUnitTester Jan 9, 2017

I'm not really glad with your tests:
Assert.Throws<AssertionException>(() => { Assert.That(from, Is.InRange(testObj, to).Using(comparer)); });
The lines seem to be for universal cases, but they are not! If testObj == from it will fail and this is an allowed option. That's only because things are mixed that should not. I will come back.
That's the next point. The number of testcases is too small. You have only one in the middle.
from = 1, test = 30, to = 46
What about the cases test = from and test = to? And you also need tests beside the from and to.
You have tried to put these two principal cases (inside range and outside range) in one test. But each case should be separately. Positive and negative tests should not be merged. Thats my opinion.

NUnitTester commented Jan 9, 2017

I'm not really glad with your tests:
Assert.Throws<AssertionException>(() => { Assert.That(from, Is.InRange(testObj, to).Using(comparer)); });
The lines seem to be for universal cases, but they are not! If testObj == from it will fail and this is an allowed option. That's only because things are mixed that should not. I will come back.
That's the next point. The number of testcases is too small. You have only one in the middle.
from = 1, test = 30, to = 46
What about the cases test = from and test = to? And you also need tests beside the from and to.
You have tried to put these two principal cases (inside range and outside range) in one test. But each case should be separately. Positive and negative tests should not be merged. Thats my opinion.

@JustinRChou

This comment has been minimized.

Show comment
Hide comment
@JustinRChou

JustinRChou Jan 9, 2017

Contributor

At that time, I was only thinking about testing the new constructor.
I didn't think it was necessary to do a coverage test since that shifts the focus to the comparer.

Contributor

JustinRChou commented Jan 9, 2017

At that time, I was only thinking about testing the new constructor.
I didn't think it was necessary to do a coverage test since that shifts the focus to the comparer.

@NUnitTester

This comment has been minimized.

Show comment
Hide comment
@NUnitTester

NUnitTester Jan 10, 2017

Nevertheless, positive and negative tests are different cases and should never be merged in one case. The approach you have made is quite well with test cases, but try to add one with always the same object, from = to = test. The test is not written universal but used so.

NUnitTester commented Jan 10, 2017

Nevertheless, positive and negative tests are different cases and should never be merged in one case. The approach you have made is quite well with test cases, but try to add one with always the same object, from = to = test. The test is not written universal but used so.

@NUnitTester

This comment has been minimized.

Show comment
Hide comment
@NUnitTester

NUnitTester Jan 10, 2017

I have had a look at the implementation:

` private ComparisonAdapter comparer = ComparisonAdapter.Default;

    /// <summary>
    /// Initializes a new instance of the <see cref="RangeConstraint"/> class.
    /// </summary>
    /// <remarks>from must be less than or equal to true</remarks> 
    /// <param name="from">Inclusive beginning of the range. Must be less than or equal to to.</param>
    /// <param name="to">Inclusive end of the range. Must be greater than or equal to from.</param>
    public RangeConstraint(IComparable from, IComparable to) : base( from, to )
    {
        // Issue #21 - https://github.com/nunit/nunit-framework/issues/21
        // from must be less than or equal to to
        if (comparer.Compare(from, to) > 0)
            throw new ArgumentException( "from must be less than to" );

        this.from = from;
        this.to = to;
    }

`

The constructors are different. This one does test from < to, the others don't.

Write an test with a class implementing IComparable and use Using with an reverse IComparable implementation. You will see, the copied constructor will fail. As I have said in the beginning, it is not allowed to do the from-to check in the constructor at all.

NUnitTester commented Jan 10, 2017

I have had a look at the implementation:

` private ComparisonAdapter comparer = ComparisonAdapter.Default;

    /// <summary>
    /// Initializes a new instance of the <see cref="RangeConstraint"/> class.
    /// </summary>
    /// <remarks>from must be less than or equal to true</remarks> 
    /// <param name="from">Inclusive beginning of the range. Must be less than or equal to to.</param>
    /// <param name="to">Inclusive end of the range. Must be greater than or equal to from.</param>
    public RangeConstraint(IComparable from, IComparable to) : base( from, to )
    {
        // Issue #21 - https://github.com/nunit/nunit-framework/issues/21
        // from must be less than or equal to to
        if (comparer.Compare(from, to) > 0)
            throw new ArgumentException( "from must be less than to" );

        this.from = from;
        this.to = to;
    }

`

The constructors are different. This one does test from < to, the others don't.

Write an test with a class implementing IComparable and use Using with an reverse IComparable implementation. You will see, the copied constructor will fail. As I have said in the beginning, it is not allowed to do the from-to check in the constructor at all.

@NUnitTester

This comment has been minimized.

Show comment
Hide comment
@NUnitTester

NUnitTester Jan 10, 2017

The earliest position seems to me to be the three methods like
public RangeConstraint Using(IComparer comparer)
to the the check if from < to, but only called if Using is used.
So, it must be done later.

NUnitTester commented Jan 10, 2017

The earliest position seems to me to be the three methods like
public RangeConstraint Using(IComparer comparer)
to the the check if from < to, but only called if Using is used.
So, it must be done later.

@NUnitTester

This comment has been minimized.

Show comment
Hide comment
@NUnitTester

NUnitTester Jan 10, 2017

Hi @ChrisMaddock: Sorry, I don't want to close it!

NUnitTester commented Jan 10, 2017

Hi @ChrisMaddock: Sorry, I don't want to close it!

@JustinRChou

This comment has been minimized.

Show comment
Hide comment
@JustinRChou

JustinRChou Jan 10, 2017

Contributor

That is a good point in moving the check from the constructor into the Using.
@nunit/core-team
Would it be good to move

if (comparer.Compare(from, to) > 0)
                throw new ArgumentException( "from must be less than to" );

into the Using methods and maybe ApplyTo as well?
I really don't know about removing the IComparable constructors though.

Contributor

JustinRChou commented Jan 10, 2017

That is a good point in moving the check from the constructor into the Using.
@nunit/core-team
Would it be good to move

if (comparer.Compare(from, to) > 0)
                throw new ArgumentException( "from must be less than to" );

into the Using methods and maybe ApplyTo as well?
I really don't know about removing the IComparable constructors though.

@rprouse rprouse added this to the 3.7 milestone May 18, 2017

@rprouse rprouse removed this from the 3.7 milestone May 29, 2017

ChrisMaddock added a commit that referenced this issue Aug 24, 2017

Merge pull request #1855 from JustinRChou/Issue-1674
#1674 Using InRange with objects and no IComparable

@ChrisMaddock ChrisMaddock added this to the 3.8 milestone Aug 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment