Skip to content

Implement Get-Random -Count without specifying an InputObject list #9111

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.

Sign up for GitHub

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

Merged
iSazonov merged 2 commits into from
Mar 26, 2019
Merged

Implement Get-Random -Count without specifying an InputObject list #9111

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3fd48e6
Improve Get-Random by allowing -Count without input objects list
throwaway774 Mar 16, 2019
2b98988
Addressing PR comments
throwaway774 Mar 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 22 additions & 13 deletions src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetRandomCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ private double ConvertToDouble(object o, double defaultIfNull)
/// <summary>
/// Number of items to output (number of list items or of numbers).
/// </summary>
[Parameter(ParameterSetName = GetRandomCommand.RandomListItemParameterSet)]
[Parameter]
[ValidateRange(1, int.MaxValue)]
public int Count { get; set; } = 1;

Expand Down Expand Up @@ -397,11 +397,14 @@ protected override void BeginProcessing()
ThrowMinGreaterThanOrEqualMax(minValue, maxValue);
}

int randomNumber = Generator.Next(minValue, maxValue);
Debug.Assert(minValue <= randomNumber, "lower bound <= random number");
Debug.Assert(randomNumber < maxValue, "random number < upper bound");
for (int i = 0; i < Count; i++)
{
int randomNumber = Generator.Next(minValue, maxValue);
Debug.Assert(minValue <= randomNumber, "lower bound <= random number");
Debug.Assert(randomNumber < maxValue, "random number < upper bound");

WriteObject(randomNumber);
WriteObject(randomNumber);
}
}
else if ((IsInt64(maxOperand) || IsInt(maxOperand)) && (IsInt64(minOperand) || IsInt(minOperand)))
{
Expand All @@ -413,11 +416,14 @@ protected override void BeginProcessing()
ThrowMinGreaterThanOrEqualMax(minValue, maxValue);
}

Int64 randomNumber = GetRandomInt64(minValue, maxValue);
Debug.Assert(minValue <= randomNumber, "lower bound <= random number");
Debug.Assert(randomNumber < maxValue, "random number < upper bound");
for (int i = 0; i < Count; i++)
{
Int64 randomNumber = GetRandomInt64(minValue, maxValue);
Debug.Assert(minValue <= randomNumber, "lower bound <= random number");
Debug.Assert(randomNumber < maxValue, "random number < upper bound");

WriteObject(randomNumber);
WriteObject(randomNumber);
}
}
else
{
Expand All @@ -429,11 +435,14 @@ protected override void BeginProcessing()
ThrowMinGreaterThanOrEqualMax(minValue, maxValue);
}

double randomNumber = GetRandomDouble(minValue, maxValue);
Debug.Assert(minValue <= randomNumber, "lower bound <= random number");
Debug.Assert(randomNumber < maxValue, "random number < upper bound");
for (int i = 0; i < Count; i++)
{
double randomNumber = GetRandomDouble(minValue, maxValue);
Debug.Assert(minValue <= randomNumber, "lower bound <= random number");
Debug.Assert(randomNumber < maxValue, "random number < upper bound");

WriteObject(randomNumber);
WriteObject(randomNumber);
}
}
}
else if (EffectiveParameterSet == MyParameterSet.RandomListItem)
Expand Down
38 changes: 27 additions & 11 deletions test/powershell/Modules/Microsoft.PowerShell.Utility/Get-Random.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,14 +59,29 @@ Describe "Get-Random DRT Unit Tests" -Tags "CI" {
)

# minimum is always set to the actual low end of the range, details refer to closed issue #887.
It "get a correct random number for '<Name>'" -TestCases $testData {
It "Should return a correct random number for '<Name>'" -TestCases $testData {
param($maximum, $minimum, $greaterThan, $lessThan, $type)

$result = Get-Random -Maximum $maximum -Minimum $minimum
$result | Should -BeGreaterThan $greaterThan
$result | Should -BeLessThan $lessThan
$result | Should -BeOfType $type
}

It "Should return correct random numbers for '<Name>' with Count specified" -TestCases $testData {
param($maximum, $minimum, $greaterThan, $lessThan, $type)

$result = Get-Random -Maximum $maximum -Minimum $minimum -Count 1
$result | Should -BeGreaterThan $greaterThan
$result | Should -BeLessThan $lessThan
Copy link
Collaborator

@iSazonov iSazonov Mar 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking asserts above it seems it should be BeLessOrEqual in all tests. What do you think?

Copy link
Author

@pougetat pougetat Mar 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you. I've made the modifications.

$result | Should -BeOfType $type

$result = Get-Random -Maximum $maximum -Minimum $minimum -Count 3
foreach ($randomNumber in $result) {
$randomNumber | Should -BeGreaterThan $greaterThan
$randomNumber | Should -BeLessThan $lessThan
$randomNumber | Should -BeOfType $type
}
}

It "Should be able to throw error when '<Name>'" -TestCases $testDataForError {
Expand Down Expand Up @@ -111,7 +126,7 @@ Describe "Get-Random" -Tags "CI" {

It "Should return a number from 1,2,3,5,8,13 " {
$randomNumber = Get-Random -InputObject 1, 2, 3, 5, 8, 13
$randomNumber | Should -Be (1 -or 2 -or 3 -or 5 -or 8 -or 13)
$randomNumber | Should -BeIn 1, 2, 3, 5, 8, 13
}

It "Should return an array " {
Expand All @@ -123,21 +138,21 @@ Describe "Get-Random" -Tags "CI" {
It "Should return three random numbers for array of 1,2,3,5,8,13 " {
$randomNumber = Get-Random -InputObject 1, 2, 3, 5, 8, 13 -Count 3
$randomNumber.Count | Should -Be 3
$randomNumber[0] | Should -Be (1 -or 2 -or 3 -or 5 -or 8 -or 13)
$randomNumber[1] | Should -Be (1 -or 2 -or 3 -or 5 -or 8 -or 13)
$randomNumber[2] | Should -Be (1 -or 2 -or 3 -or 5 -or 8 -or 13)
$randomNumber[0] | Should -BeIn 1, 2, 3, 5, 8, 13
$randomNumber[1] | Should -BeIn 1, 2, 3, 5, 8, 13
$randomNumber[2] | Should -BeIn 1, 2, 3, 5, 8, 13
$randomNumber[3] | Should -BeNullOrEmpty
}

It "Should return all the numbers for array of 1,2,3,5,8,13 in no particular order" {
$randomNumber = Get-Random -InputObject 1, 2, 3, 5, 8, 13 -Count ([int]::MaxValue)
$randomNumber.Count | Should -Be 6
$randomNumber[0] | Should -Be (1 -or 2 -or 3 -or 5 -or 8 -or 13)
$randomNumber[1] | Should -Be (1 -or 2 -or 3 -or 5 -or 8 -or 13)
$randomNumber[2] | Should -Be (1 -or 2 -or 3 -or 5 -or 8 -or 13)
$randomNumber[3] | Should -Be (1 -or 2 -or 3 -or 5 -or 8 -or 13)
$randomNumber[4] | Should -Be (1 -or 2 -or 3 -or 5 -or 8 -or 13)
$randomNumber[5] | Should -Be (1 -or 2 -or 3 -or 5 -or 8 -or 13)
$randomNumber[0] | Should -BeIn 1, 2, 3, 5, 8, 13
$randomNumber[1] | Should -BeIn 1, 2, 3, 5, 8, 13
$randomNumber[2] | Should -BeIn 1, 2, 3, 5, 8, 13
$randomNumber[3] | Should -BeIn 1, 2, 3, 5, 8, 13
$randomNumber[4] | Should -BeIn 1, 2, 3, 5, 8, 13
$randomNumber[5] | Should -BeIn 1, 2, 3, 5, 8, 13
$randomNumber[6] | Should -BeNullOrEmpty
}

Expand All @@ -163,6 +178,7 @@ Describe "Get-Random" -Tags "CI" {
$secondRandomNumber = Get-Random 34359738367 -SetSeed 20
$firstRandomNumber | Should -Be @secondRandomNumber
}

It "Should throw an error because the hexadecimal number is to large " {
{ Get-Random 0x07FFFFFFFFFFFFFFFF } | Should -Throw "Value was either too large or too small for a UInt32"
Copy link
Contributor

@mklement0 mklement0 Mar 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't dug deeper, but the full error message is self-contradictory (Int32 vs. UInt32): Cannot convert value "0x07FFFFFFFFFFFFFFFF" to type "System.Int32". Error: "Value was either too large or too small for a UInt32." Where does the UInt come from?

Copy link
Collaborator

@vexx32 vexx32 Mar 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I actually fixed this back when I was toying with the tokenizer, and then it was requested I re-break this for consistency with prior implementations. This can be fixed, but the fix would be in the tokenizer and how hex numbers get parsed.

This number is an extra digit beyond the widest permitted for uint64, and a hex number with a leading zero is forced to be an unsigned numeral when using the conversion paths we have in the tokenizer (which are still done with Convert.ToInt32() I think?). I think the default path goes to (u)int32 because it checks for (u)int64 lengths and then defaults to (u)int32 for all other cases iirc.

I remember I originally did some fun things in my tokenizer PRs where I would automatically upcast between uint/int64 if needed, but it differed too much from the previous implementation I think and we decided to keep the behaviour consistent. I think my solution was to have it upcast up to decimal (or even biginteger) if needed, which worked well.

I think due to the refactoring in #7993 this error message might actually change, but it's been a little while and I don't quite recall.

Copy link
Contributor

@mklement0 mklement0 Mar 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see: if the pre-parameter-binding typing can't convert a "number-looking" argument to an actual number, it passes a string, and line baseObject = System.Management.Automation.Language.Parser.ScanNumber((string)baseObject, typeof(int)); then re-attempts conversion, which fails.

Again, not in the scope of this PR, but I wonder if the error message could be made friendlier (and it's still curious that UInt32 is referenced, given that hex. literals seem to top out at Int64 [see previous comment]).

Copy link
Contributor

@mklement0 mklement0 Mar 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, @vexx32 - wires crossed - reading your comment now.

}
Expand Down