Skip to content

Allow RangeAttribute to be specified multiple times for the same argument #2469

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

Closed
jongleur1983 opened this issue Oct 3, 2017 · 5 comments
Closed

Comments

@jongleur1983
Copy link

The RangeAttribute (e.g. [Range(0, 20)]) is a way to define the value range to test for in a theory, but it's restricted to occur only once per parameter of the test method.

Sometimes the same test code should be tested for different intervals.
Therefore it would be good to allow multiple ranges to be specified for the same parameter:

public void testcode([Range(-10, 1), Range(20, 25)] value)
{
  // do something
}```
@rprouse rprouse added the is:idea label Oct 3, 2017
@rprouse
Copy link
Member

rprouse commented Oct 3, 2017

As a workaround, you could use a TestCaseSource for this.

I am going to mark as an idea for discussion. I can see your use-case and it is valid, but there are alternatives and I suspect the feature would be used so little that the added complexity to the code and potential for bugs would outweigh its usefulness.

We also need to look at the RangeAttribute and see how it generates test cases so we can see how easy it would be to allow multiple. If it is easy, the balance above may change.

@CharliePoole
Copy link
Member

@rprouse It may not be all that complicated.

RangeAttribute doesn't have any AttributeUsage specified, because it derives from ValuesAttribute. ValuesAttribute has AllowsMultiple set to false simply because there appeared to be no reason to do otherwise. When you use [Values(...)], you can just put all the values into the argument, so there's no need for more than one instance of the attribute.

OTOH, there would be no harm in allowing multiple instances of ValuesAttribute as far as I can see, so that's one way to fix the problem.

Another easy way out would be to stop deriving RangeAttribute from 'ValuesAttribute`. It would cause a very small amount of code duplication and would actually be more consistent with our general approach of making each attribute independent.

The key issue is whether multiple parameter data sources can be used on a single parameter, with NUnit combining the values from each source into a single list. The answer is "yes" - we already do that.

I'd call this an easyfix and suggest sombody should just try one of the above approaches.

@rprouse
Copy link
Member

rprouse commented Oct 3, 2017

@CharliePoole sounds good, thanks for looking at the code. I am going to switch this to easy-fix 'hoping' that it is. If anyone picks this up, please discuss if it becomes more difficult than expected.

@pknouff
Copy link
Contributor

pknouff commented Oct 6, 2017

I'd like to tackle this one.

@mikkelbu
Copy link
Member

mikkelbu commented Oct 6, 2017

Sounds great @pknouff. @rprouse can you invite @pknouff to the project, so that we can assign him to the issue.

pknouff added a commit to pknouff/nunit that referenced this issue Oct 6, 2017
Previously, `RangeAttribute` derived from `ValuesAttribute`. However,
there are certain use cases which apply to the `RangeAttribute`, but not
to the `ValuesAttribute`. For example, we may want specify multiple,
distinct ranges for a parameter by adding the `RangeAttribute` multiple
times, but there isn't really a corresponding use case for the values
attribute. We could just specify all the desired values in on attribute.

This change updates the `RangeAttribute` to no longer derive from the
`ValuesAttribute`. Instead, it now derives from `DataAttribute` and
implements `IParameterDataSource`. The `IParameterDataSource.GetData`
method has been implemented in `RangeAttribute` to preserve the behavior
of that method in the `ValuesAttribute`. However, certain cases that
could not be hit in the `RangeAttribute` (e.g. when the internal data
was non-numeric) were removed.

Fixes nunit#2469
pknouff added a commit to pknouff/nunit that referenced this issue Oct 6, 2017
Previously, `RangeAttribute` derived from `ValuesAttribute`. However,
there are certain use cases which apply to the `RangeAttribute`, but not
to the `ValuesAttribute`. For example, we may want specify multiple,
distinct ranges for a parameter by adding the `RangeAttribute` multiple
times, but there isn't really a corresponding use case for the values
attribute. We could just specify all the desired values in on attribute.

This change updates the `RangeAttribute` to no longer derive from the
`ValuesAttribute`. Instead, it now derives from `DataAttribute` and
implements `IParameterDataSource`. The `IParameterDataSource.GetData`
method has been implemented in `RangeAttribute` to preserve the behavior
of that method in the `ValuesAttribute`. However, certain cases that
could not be hit in the `RangeAttribute` (e.g. when the internal data
was non-numeric) were removed.

Fixes nunit#2469
@ChrisMaddock ChrisMaddock added this to the 3.9 milestone Oct 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants