Skip to content

Commit fee7384

Browse files
AArnottAArnott
committed
Fix random value generation issues:
1. Non-unique values may be generated, leading to fewer unique cases than requested. 2. The `maxValue` constructor argument wasn't actually interpreted as in *inclusive* maximum, but its exclusive nature was not documented either. 3. Too many constructor overloads that were not clear when reading users. Switched to named arguments pattern.
1 parent ba0b795 commit fee7384

File tree

6 files changed

+138
-116
lines changed

6 files changed

+138
-116
lines changed

‎src/Directory.Build.props

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
<Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration>
44
<BaseIntermediateOutputPath>$(MSBuildThisFileDirectory)..\obj\$(MSBuildProjectName)\</BaseIntermediateOutputPath>
55
<OutputPath>$(MSBuildThisFileDirectory)..\bin\$(MSBuildProjectName)\$(Configuration)\</OutputPath>
6+
<LangVersion>8</LangVersion>
67
<DebugType Condition=" '$(Configuration)' == 'Debug' ">full</DebugType>
78
<DebugType Condition=" '$(Configuration)' == 'Release' ">pdbonly</DebugType>
89
</PropertyGroup>

‎src/Xunit.Combinatorial.Tests/CombinatorialRandomAttributeTests.cs

Lines changed: 24 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -15,71 +15,71 @@ public void ConstructorNoParams()
1515
[InlineData(3)]
1616
[InlineData(25)]
1717
public void ConstructorCount(int count)
18-
=> Check(new CombinatorialRandomAttribute(count), count);
18+
=> Check(new CombinatorialRandomAttribute { Count = count });
1919

2020
[Theory]
2121
[InlineData(1, 25)]
2222
[InlineData(3, 100)]
2323
[InlineData(24, 1000)]
2424
public void ConstructorCountMaxValue(int count, int maxValue)
25-
=> Check(new CombinatorialRandomAttribute(count, maxValue), count, maxValue: maxValue);
25+
=> Check(new CombinatorialRandomAttribute { Count = count, Maximum = maxValue });
2626

2727
[Theory]
2828
[InlineData(1, 25, 35)]
2929
[InlineData(3, 100, 200)]
30-
[InlineData(24, 1000, 1001)]
3130
public void ConstructorCountMinMaxValues(int count, int minValue, int maxValue)
32-
=> Check(new CombinatorialRandomAttribute(count, minValue, maxValue), count, minValue, maxValue);
31+
=> Check(new CombinatorialRandomAttribute { Count = count, Minimum = minValue, Maximum = maxValue });
3332

3433
[Theory]
3534
[InlineData(1, 25, 35, 234)]
3635
[InlineData(3, 100, 200, 1343)]
3736
[InlineData(24, 1000, 3000, 56732)]
38-
[InlineData(12, 576, 765, CombinatorialRandomAttribute.NoSeed)]
37+
[InlineData(12, 576, 765, 0)]
3938
public void ConstructorCountMinMaxValuesSeed(int count, int minValue, int maxValue, int seed)
40-
=> Check(new CombinatorialRandomAttribute(count, minValue, maxValue, seed), count, minValue, maxValue, seed);
39+
=> Check(new CombinatorialRandomAttribute { Count = count, Minimum = minValue, Maximum = maxValue, Seed = seed });
4140

4241
[Theory]
4342
[InlineData(0)]
4443
[InlineData(-1)]
4544
[InlineData(int.MinValue)]
4645
public void ConstructorOutOfRangeCount(int count)
4746
{
48-
Assert.Throws<ArgumentOutOfRangeException>(() => new CombinatorialRandomAttribute(count));
49-
Assert.Throws<ArgumentOutOfRangeException>(() => new CombinatorialRandomAttribute(count, 100));
50-
Assert.Throws<ArgumentOutOfRangeException>(() => new CombinatorialRandomAttribute(count, 0, 100));
51-
Assert.Throws<ArgumentOutOfRangeException>(() => new CombinatorialRandomAttribute(count, 0, 100, 325));
52-
Assert.Throws<ArgumentOutOfRangeException>(() => new CombinatorialRandomAttribute(count, 0, 100, CombinatorialRandomAttribute.NoSeed));
47+
Assert.Throws<InvalidOperationException>(() => new CombinatorialRandomAttribute { Count = count }.Values);
5348
}
5449

55-
internal void Check(
56-
CombinatorialRandomAttribute attribute,
57-
int count = CombinatorialRandomAttribute.DefaultCount,
58-
int minValue = CombinatorialRandomAttribute.DefaultMinValue,
59-
int maxValue = CombinatorialRandomAttribute.DefaultMaxValue,
60-
int seed = CombinatorialRandomAttribute.NoSeed)
50+
[Fact]
51+
public void CountHigherThanRangeSize()
52+
{
53+
Assert.Throws<InvalidOperationException>(() => new CombinatorialRandomAttribute { Count = 6, Minimum = 1, Maximum = 5 }.Values);
54+
Assert.Throws<InvalidOperationException>(() => new CombinatorialRandomAttribute { Count = 4, Minimum = -3, Maximum = -1 }.Values);
55+
_ = new CombinatorialRandomAttribute { Count = 3, Minimum = 1, Maximum = 3 }.Values;
56+
_ = new CombinatorialRandomAttribute { Count = 3, Minimum = -3, Maximum = -1 }.Values;
57+
}
58+
59+
internal void Check(CombinatorialRandomAttribute attribute)
6160
{
6261
Assert.NotNull(attribute.Values);
63-
Assert.Equal(count, attribute.Values.Length);
62+
Assert.Equal(attribute.Count, attribute.Values.Length);
6463

6564
Assert.All(attribute.Values, value =>
6665
{
6766
Assert.IsType<int>(value);
6867
var intValue = (int)value;
69-
Assert.InRange(intValue, minValue, maxValue - 1);
68+
Assert.InRange(intValue, attribute.Minimum, attribute.Maximum);
7069
});
7170

72-
if (seed != CombinatorialRandomAttribute.NoSeed)
71+
if (attribute.Seed != CombinatorialRandomAttribute.NoSeed)
7372
{
74-
Assert.Equal(RandomIterator(count, new Random(seed), minValue, maxValue), attribute.Values.Cast<int>());
73+
Assert.Equal(RandomIterator(new Random(attribute.Seed), attribute.Minimum, attribute.Maximum).Distinct().Take(attribute.Count).ToArray(), attribute.Values.Cast<int>());
7574
}
7675
}
7776

78-
internal static IEnumerable<int> RandomIterator(int count, Random random, int minValue, int maxValue)
77+
internal static IEnumerable<int> RandomIterator(Random random, int minValue, int maxValue)
7978
{
80-
for (int i = 0; i < count; i++)
79+
while (true)
8180
{
82-
yield return random.Next(minValue, maxValue);
81+
int value = random.Next(minValue, maxValue + 1);
82+
yield return value;
8383
}
8484
}
8585
}

‎src/Xunit.Combinatorial.Tests/SampleUses.cs

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -71,31 +71,31 @@ public void CombinatorialCustomRange([CombinatorialRange(0, 5)] int p1, [Combina
7171
[Theory, CombinatorialData]
7272
public void CombinatorialRandomValuesDefault([CombinatorialRandom()] int p1)
7373
{
74-
Assert.InRange(p1, 0, int.MaxValue - 1);
74+
Assert.InRange(p1, 0, int.MaxValue);
7575
}
7676

7777
[Theory, CombinatorialData]
78-
public void CombinatorialRandomValuesCount([CombinatorialRandom(10)] int p1)
78+
public void CombinatorialRandomValuesCount([CombinatorialRandom(Count = 10)] int p1)
7979
{
80-
Assert.InRange(p1, 0, int.MaxValue - 1);
80+
Assert.InRange(p1, 0, int.MaxValue);
8181
}
8282

8383
[Theory, CombinatorialData]
84-
public void CombinatorialRandomValuesCountMaxValue([CombinatorialRandom(10, 35)] int p1)
84+
public void CombinatorialRandomValuesCountMaxValue([CombinatorialRandom(Count = 10, Maximum = 35)] int p1)
8585
{
86-
Assert.InRange(p1, 0, 35 - 1);
86+
Assert.InRange(p1, 0, 35);
8787
}
8888

8989
[Theory, CombinatorialData]
90-
public void CombinatorialRandomValuesCountMinMaxValues([CombinatorialRandom(10, -20, -5)] int p1)
90+
public void CombinatorialRandomValuesCountMinMaxValues([CombinatorialRandom(Count = 10, Minimum = -20, Maximum = -5)] int p1)
9191
{
92-
Assert.InRange(p1, -20, -5 - 1);
92+
Assert.InRange(p1, -20, -5);
9393
}
9494

9595
[Theory, CombinatorialData]
96-
public void CombinatorialRandomValuesCountMinMaxValuesSeed([CombinatorialRandom(10, -5, 6, 567)] int p1)
96+
public void CombinatorialRandomValuesCountMinMaxValuesSeed([CombinatorialRandom(Count = 10, Minimum = -5, Maximum = 6, Seed = 567)] int p1)
9797
{
98-
Assert.InRange(p1, -5, 6 - 1);
98+
Assert.InRange(p1, -5, 6);
9999
}
100100

101101
[AttributeUsage(AttributeTargets.Parameter)]

‎src/Xunit.Combinatorial/CombinatorialRandomAttribute.cs

Lines changed: 56 additions & 83 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@
33
namespace Xunit
44
{
55
using System;
6+
using System.Collections.Generic;
7+
using System.Globalization;
68

79
/// <summary>
810
/// Specifies which range of values for this parameter should be used for running the test method.
@@ -11,116 +13,87 @@ namespace Xunit
1113
public class CombinatorialRandomAttribute : Attribute
1214
{
1315
/// <summary>
14-
/// Default quantity of values to generate.
16+
/// Special seed value to create System.Random class without seed.
1517
/// </summary>
16-
public const int DefaultCount = 5;
18+
public const int NoSeed = 0;
1719

18-
/// <summary>
19-
/// Default minValue for System.Random.Next method.
20-
/// </summary>
21-
public const int DefaultMinValue = 0;
20+
private object[] values;
2221

2322
/// <summary>
24-
/// Default maxValue for System.Random.Next method.
23+
/// Gets or sets the number of values to generate. Must be positive.
2524
/// </summary>
26-
public const int DefaultMaxValue = int.MaxValue;
25+
/// <value>The default value is 5.</value>
26+
public int Count { get; set; } = 5;
2727

2828
/// <summary>
29-
/// Special seed value to create System.Random class without seed.
29+
/// Gets or sets the minimum value (inclusive) that may be randomly generated.
3030
/// </summary>
31-
public const int NoSeed = int.MinValue;
31+
/// <value>The default value is 0.</value>
32+
public int Minimum { get; set; }
3233

3334
/// <summary>
34-
/// Initializes a new instance of the <see cref="CombinatorialRandomAttribute"/> class.
35+
/// Gets or sets the maximum value (inclusive) that may be randomly generated.
3536
/// </summary>
36-
public CombinatorialRandomAttribute()
37-
: this(DefaultCount, DefaultMinValue, DefaultMaxValue, NoSeed)
38-
{
39-
}
37+
/// <value>The default value is <c><see cref="int.MaxValue"/> - 1</c>, which is the maximum allowable value.</value>
38+
public int Maximum { get; set; } = int.MaxValue - 1;
4039

4140
/// <summary>
42-
/// Initializes a new instance of the <see cref="CombinatorialRandomAttribute"/> class.
41+
/// Gets or sets the seed to use for random number generation.
4342
/// </summary>
44-
/// <param name="count">
45-
/// The quantity of values to generate.
46-
/// Cannot be less than 1, which would conceptually result in zero test cases.
47-
/// </param>
48-
public CombinatorialRandomAttribute(int count)
49-
: this(count, DefaultMinValue, DefaultMaxValue, NoSeed)
50-
{
51-
}
43+
/// <value>The default value of <see cref="NoSeed"/> allows for a new seed to be used each time.</value>
44+
public int Seed { get; set; } = NoSeed;
5245

5346
/// <summary>
54-
/// Initializes a new instance of the <see cref="CombinatorialRandomAttribute"/> class.
47+
/// Gets the values that should be passed to this parameter on the test method.
5548
/// </summary>
56-
/// <param name="count">
57-
/// The quantity of values to generate.
58-
/// Cannot be less than 1, which would conceptually result in zero test cases.
59-
/// </param>
60-
/// <param name="maxValue">
61-
/// maxValue for System.Random.Next method.
62-
/// </param>
63-
public CombinatorialRandomAttribute(int count, int maxValue)
64-
: this(count, DefaultMinValue, maxValue, NoSeed)
65-
{
66-
}
49+
/// <value>An array of values.</value>
50+
public object[] Values => this.values ??= this.GenerateValues();
6751

68-
/// <summary>
69-
/// Initializes a new instance of the <see cref="CombinatorialRandomAttribute"/> class.
70-
/// </summary>
71-
/// <param name="count">
72-
/// The quantity of values to generate.
73-
/// Cannot be less than 1, which would conceptually result in zero test cases.
74-
/// </param>
75-
/// <param name="minValue">
76-
/// minValue for System.Random.Next method.
77-
/// </param>
78-
/// <param name="maxValue">
79-
/// maxValue for System.Random.Next method.
80-
/// </param>
81-
public CombinatorialRandomAttribute(int count, int minValue, int maxValue)
82-
: this(count, minValue, maxValue, NoSeed)
52+
private object[] GenerateValues()
8353
{
84-
}
54+
if (this.Count < 1)
55+
{
56+
throw new InvalidOperationException(string.Format(CultureInfo.CurrentCulture, Strings.ValueMustBePositive, nameof(this.Count)));
57+
}
8558

86-
/// <summary>
87-
/// Initializes a new instance of the <see cref="CombinatorialRandomAttribute"/> class.
88-
/// </summary>
89-
/// <param name="count">
90-
/// The quantity of values to generate.
91-
/// Cannot be less than 1, which would conceptually result in zero test cases.
92-
/// </param>
93-
/// <param name="minValue">
94-
/// minValue for System.Random.Next method.
95-
/// </param>
96-
/// <param name="maxValue">
97-
/// maxValue for System.Random.Next method.
98-
/// </param>
99-
/// <param name="seed">
100-
/// Seed for System.Random constructor. If Seed equal to NoSeed value then System.Random constructor whithout seed used.
101-
/// </param>
102-
public CombinatorialRandomAttribute(int count, int minValue, int maxValue, int seed)
103-
{
104-
if (count < 1)
59+
if (this.Minimum > this.Maximum)
60+
{
61+
throw new InvalidOperationException(string.Format(CultureInfo.CurrentCulture, Strings.XMustNotBeGreaterThanY, nameof(this.Minimum), nameof(this.Maximum)));
62+
}
63+
64+
int maxPossibleValues = this.Maximum - this.Minimum + 1;
65+
if (this.Count > maxPossibleValues)
10566
{
106-
throw new ArgumentOutOfRangeException(nameof(count));
67+
throw new InvalidOperationException(string.Format(CultureInfo.CurrentCulture, Strings.MoreRandomValuesRequestedThanPossibleOnes, nameof(this.Count), nameof(this.Minimum), nameof(this.Maximum)));
10768
}
10869

109-
var random = seed == NoSeed ? new Random() : new Random(seed);
70+
Random random = this.Seed != NoSeed ? new Random(this.Seed) : new Random();
11071

111-
object[] values = new object[count];
112-
for (int i = 0; i < count; i++)
72+
HashSet<int> collisionChecker = new HashSet<int>();
73+
object[] values = new object[this.Count];
74+
int collisionCount = 0;
75+
int i = 0;
76+
while (collisionChecker.Count < this.Count)
11377
{
114-
values[i] = random.Next(minValue, maxValue);
78+
int value = random.Next(this.Minimum, this.Maximum + 1);
79+
if (collisionChecker.Add(value))
80+
{
81+
values[i++] = value;
82+
}
83+
else
84+
{
85+
collisionCount++;
86+
}
87+
88+
if (collisionCount > collisionChecker.Count * 5)
89+
{
90+
// We have collided in random values far more than we have successfully generated values.
91+
// Rather than spin in this loop, throw.
92+
throw new InvalidOperationException(Strings.TooManyRandomCollisions);
93+
}
11594
}
11695

117-
this.Values = values;
96+
return values;
11897
}
119-
120-
/// <summary>
121-
/// Gets the values that should be passed to this parameter on the test method.
122-
/// </summary>
123-
/// <value>An array of values.</value>
124-
public object[] Values { get; }
12598
}
12699
}

‎src/Xunit.Combinatorial/Strings.Designer.cs

Lines changed: 36 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎src/Xunit.Combinatorial/Strings.resx

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,19 @@
132132
<data name="InternalExceptionMessage" xml:space="preserve">
133133
<value>An internal error occurred. Please contact customer support.</value>
134134
</data>
135+
<data name="MoreRandomValuesRequestedThanPossibleOnes" xml:space="preserve">
136+
<value>{0} must not exceed the length of the range from {1} to {2}.</value>
137+
</data>
135138
<data name="ServiceMissing" xml:space="preserve">
136139
<value>Cannot find an instance of the {0} service.</value>
137140
</data>
141+
<data name="TooManyRandomCollisions" xml:space="preserve">
142+
<value>We are unable to generate the desired number of unique random values because too many non-unique random numbers are coming from the random number generator. Try reducing your target count or expanding your allowed range.</value>
143+
</data>
144+
<data name="ValueMustBePositive" xml:space="preserve">
145+
<value>The value for {0} must be positive.</value>
146+
</data>
147+
<data name="XMustNotBeGreaterThanY" xml:space="preserve">
148+
<value>The value of {0} must not be greater than the value of {1}.</value>
149+
</data>
138150
</root>

0 commit comments

Comments
 (0)