Allow option on RandomAttribute to produce distinct values

[NUnitTester]

RandomAttribute produces not distinct values. So NUnit Adapter 3.7.0.0 tells errors that test with the same name does already exists.

public void MyTest([Random(0, 9, 7)] int x)

The range is small in this case, so it is nearly sure to get the same value twice.
I expect the values should be distinct. If the range is too small the number of random numbers to produce shopuld be reduced.

[CharliePoole]

My feeling .is that removing duplicates, while possible, would make the number non-random.

For the purposes it is used here, especially with a single argument, that might be OK, although the example seems a bit contrived. However, the underlying randomizer is used in other ways as well, where filtering for distinct values would probably not be appropriate.

What if we added a named parameter to the attribute to allow you to specify Distinct values?
Something like.... Distinct=true or Duplciates=false. What do people think? Should this be a feature?

[rprouse]

@CharliePoole I was just typing a similar response when yours came in, for cases like this, should we add a Distinct property, [Random(0, 9, 7, Distinct=true)]. I prefer distinct over duplicates because it mirrors the SQL and LINQ syntax, so it will be more intuitive to .NET developers.

[CharliePoole]

I can go with that. Whoever does it should look to see if we want to add the distinct value capablility in the attribute or make it an option of the randomizer itself, so it could be used directly from TestContext if desired.

[jnm2]

What you're basically asking for is to take from a randomly sorted list of numbers.

[NUnitTester]

Distinct=true is my preferred way. But true should be the default because the NUnit-Adapter does not like duplicates.
@CharliePoole I'm not with you: The list of numbers is random still if duplicates are eliminated. If a number is still existing, an other random number is made. So if range is large enough, randomness ist still available. In case of tests the same number as parameter should produce the same result. There are only really rare cases what makes a difference with same parameter values.
@jnm2 No, sorting is forbidden at all in random case. Only the TestCase collector manager can sort the names in some way. For implementation an HashSet with the created numbers up to now is needed.

[jnm2]

@NUnitTester it is still random, but less random because you can predict that the same number will not show up. And yes, mathematically, discarding duplicate random numbers between A and B is the same as randomly sorting the list of numbers from A to B and taking from that.

[CharliePoole]

@NUnitTester I don't agree with the true default, but even if I saw it your way, I don't see us changing many years of behavior in generating random cases in this way.

When random numbers are generated, there is an expectation that duplicates will arise at a certain frequency, which can be calculated. That frequency is never zero. So, if we eliminate duplicates, then the sequence is no longer random in any mathematical sense. It becomes a selection from a random sequence. In the case of test case generation, this would seriously bias the list of tested cases, especially when using either combinatorial or pairwise combination of the values.

In the case of using the randomizer within the test code, via TestContext.CurrentContext.Random, the impact is unpredictable, since we can't know what users are using those values for.

I can see accepting this as a work item, albeit low priority, for those who want distinct values, but I would not favor support taking it any farther than that by making it the default behavior.

IMPLEMENTATION NOTE: If the range is less than the selected numbers, the use of Distinct=true should result in an invalid test, rather than being silently ignored.

[ChrisMaddock]

I wonder if instead the NUnit Adapter should deduplicate test names returned from the framework? (e.g. by adding a unique suffix.)

I agree with @NUnitTester's point that this should 'just work' - but changing previous defaults isn't ideal.

[CharliePoole]

@NUnitTester

"true should be the default because the NUnit-Adapter does not like duplicates."

I believe you mean duplicate test names. The NUnit adapter, like all parts of NUnit, is quite happy with duplicate test names. It's the test explorer window of visual studio that doesn't like duplicate names, because the developers made the assumption that the test display name is unique for all frameworks they could imagine.

Frankly, the day we do or don't do something in NUnit because of what Visual Studio does or doesn't like is the day we should stop working on our framework. Please read our Vision statement on the wiki, particularly the part about platform- and IDE- independence!

That's not to say we can't make adjustments to Visual Studio within our Visual Studio adapter - at least to the extent that the VS API makes it possible. But attributes and how they work are a part of the NUnit framework, which treats all runners equally.

[CharliePoole]

@ChrisMaddock I think it does "just work" unless the OP was referring to how the adapter deals with it. That's not what the issue says, however, and I took it to mean that @NUnitTester just doesn't want to run the same test case twice. That totally makes sense, except that the way it "works" now is that he is asking for possible duplicate cases and he is getting them. This has been the way it works since we created RandomAttribute, back in 2008. "Distinct" is a valid request, but it's hard to see it as a very important need, since this is the first request for it over all that time.

OTOH, if @NUnitTester 's motivation for this request is actually to eliminate duplicates under the adapter, then this should be an adapter issue - but only after reviewing the existing issues that deal with the problem. I don't want to get too deep into it in this framework issue, but the too main options we have looked at in the adapter in the past are reduction of duplicates (or "de-duplication" via a suffix) and changing how we present tests to VS so that the methods become the test and the various sets of args simply produce different results for that "test".

I think it's very important to us that the framework not be driven in any way by what VS - or any other third-party product - wants to see.

[NUnitTester]

@CharliePoole After that discussion I agree the arguments that it should not be driven by Visual Studio. So let make a distinct options with default false. I have not thought about pairwice
And additionally the NUnit-Adapter should make the names unique if needed. This would be nice. I can see use cases where distinct is not the solution as in my case.

@jnm2 In the case of NUnit it would make only differences in rare cases. But mathematically sort on random number change the behaviour in many cases: if you look on length of running sequences. So in case of NUnit there is no need of sorting, so it is better to do not.