Skip to content
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

Documentation of the InRange constraint does not mention whether from...to also allows negative ranges, e.g. from 0 to -10 #872

Open
maettu-this opened this issue Nov 24, 2023 · 8 comments

Comments

@maettu-this
Copy link
Contributor

maettu-this commented Nov 24, 2023

Referring to https://docs.nunit.org/articles/nunit/writing-tests/constraints/RangeConstraint.html, the documentation misses information on whether from...to also allows a negative range, e.g. from 0 to -10. Why can this be important? Sometimes an assertion needs a tolerance, e.g. Is.InRange(data.Length, (data.Length + (int)(Math.Round(data.Length * tolerance)))) where tolerance can be a positive or negative value. From the documentation it is not clear whether this can possibly work, or wether to must be a value larger than from. Only looking at the code (or decompiling) reveals:

if (from.CompareTo(to) > 0)
{
	throw new ArgumentException("from must be less than to");
}

(NUnit 2.6.4.14350)

I personally would prefer that InRange allowed positive and negative ranges.
And in any case, documentation thereof should be improved.

@mikkelbu
Copy link
Member

@maettu-this The range constraint allows negative ranges, but it does not allow range specified in decreasing order, so you have to phrase it like

Assert.That(someValue, Is.InRange(-10, 0));

I your case I guess you need to add logic to determine where to place data.Length and (data.Length + (int)(Math.Round(data.Length * tolerance))) in the constraint.

I'll just see if we get other comments, but otherwise I think we should improve the documentation. I don't think we should support ranges in decreasing order as that does not make sense from a mathematical view - e.g. [0, -10] is an empty set, so the constraint would always fail. That being said I never used this constraint (or known that it existed), so I don't fell that strongly about it.

@maettu-this
Copy link
Contributor Author

@mikkelbu I can reason the argueing mathematically seen. However, in my opinion from and to not necessary need to mean min and max. Also consider the implications on the test code:

// The "InRange" constraint requires that "from must be less than to" (https://github.com/nunit/docs/issues/872):
var from = Math.Min(data.Length, (data.Length + (int)(Math.Round(data.Length * tolerance))));
var to   = Math.Max(data.Length, (data.Length + (int)(Math.Round(data.Length * tolerance))));

Assert.That(someValue, Is.InRange(from, to));

Opposed to a simpler:

Assert.That(someValue, Is.InRange(data.Length, (data.Length + (int)(Math.Round(data.Length * tolerance)))));

And also consider that enabling decreasing ranges wouldn't mean a breaking change, as all existing code is so far required to use increasing ranges, thus is not impacted.

@stevenaw
Copy link
Member

I also don't feel strongly one way or another about the behavior here, and I'd probably want to experiment a bit at a computer before forming an opinion.

If we did decide to keep the existing behavior, we may want to consider renaming the parameters to min and max to clarify intent.

@maettu-this
Copy link
Contributor Author

@stevenaw min and max would definitely underline the semantics. lower and upper would be alternatives, i.e. the lower and upper boundaries of the range. I have seen various pieces of software which use these terms with ranges. But still I'd prefer more flexibility with from and to.

@stevenaw
Copy link
Member

@maettu-this Sorry, but I'm starting to lean towards @mikkelbu suggestion as well based on a bit of experimentation with other APIs which people may be used to and expect similar behaviour.

It looks as though SQL's BETWEEN clause - or at least the MSSQL flavour of it - requires that value1 be lower than value2. Something like this will always yield an empty resultset:

WHERE BETWEEN 0 AND -10

I've also seen similar behaviour within dotnet itself. Enumerable.Range() takes a start/count instead of from/to but the method is similarly-enough named that I could see expectations for similar behaviour. It looks like it also requires a positive count and does not support decrementing sets. For example, the following will throw an ArgumentOutOfRangeException:

Enumerable.Range(0, -10)

I'll also leave this open for a bit to see if there's any other opinions from team members

@manfred-brands
Copy link
Member

My few cents.

Unit tests should be independent, i.e. stateless, so it shouldn't matter if you execute one in order 1, 2, 3 or in 3, 2, 1 or even 2, 1, 3 in parallel.

@maettu-this
Copy link
Contributor Author

maettu-this commented Nov 30, 2023

Unit tests should be independent, i.e. stateless, so it shouldn't matter if you execute one in order 1, 2, 3 or in 3, 2, 1 or even 2, 1, 3 in parallel.

@manfred-brands That was never in question. This issue is about a single assertion within a single test case.

@stevenaw stevenaw transferred this issue from nunit/nunit Dec 9, 2023
@stevenaw
Copy link
Member

stevenaw commented Dec 9, 2023

I've transferred this to the documenation repo since it sounds like we want to keep the behaviour the same as it is today

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants