-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Update format-hex tests to include -TestCase parameter #3800
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that these functions should just be inlined into the appropriate context block. Having these functions only makes things a little harder to find. it allows the test case data be closer to the actual test.
|
@MiaRomero - the assignee of a PR is the person responsible for merging to master - and only maintainers can do that merge. We assign issues to regular contributors, but not the PR. |
|
@JamesWTruher or @Francisco-Gamino - Do you guys have any other concerns or comments? |
|
@MiaRomero - We need to add you to the Microsoft organization. I just noticed that you are not a member yet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should check $output.Count >=1 or =1 not $null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove - next line make the check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, removed in all mentioned instances.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove - next line make the check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove - next line make the check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove - next line make the check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove - next line make the check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove - next line make the check.
8ef74e2 to
84a926b
Compare
|
@JamesWTruher fixed the >1 issue. The way it was before was not actually testing the contents of the second file so when I corrected it, those tests failed because the output was on two lines. I changed the input text to avoid the hassle of having two lines of output in the second expected result (then had to create a file with longer text to test the buffer underrun). |
|
|
||
| Context "Continues to Process Valid Paths" { | ||
|
|
||
| $skipTest = ([System.Management.Automation.Platform]::IsLinux -or [System.Management.Automation.Platform]::IsOSX) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be happier if this was combined with the construct in 378
IsLinux -or IsOSX -or (-not $certProviderAvailable)
then 378 could just be:
It "<Name>" -Skip:$skip
same with the code below
| } | ||
|
|
||
| Context "Path Paramater" { | ||
| Context "Path and LiteralPath Paramaters" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in Paramaters -> Parameters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
| $output = Format-Hex -LiteralPath $InvalidPath, $inputFile1 -ErrorVariable errorThrown -ErrorAction SilentlyContinue | ||
| } | ||
|
|
||
| $errorThrown.FullyQualifiedErrorId | Should Match $ExpectedFullyQualifiedErrorId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use ShouldBeErrorId, like here:
PowerShell/test/powershell/Modules/Microsoft.PowerShell.Diagnostics/New-WinEvent.Tests.ps1
Line 27 in 0b7451b
| { New-WinEvent -ProviderName NonExistingProvider -Id 0 } | ShouldBeErrorId 'System.ArgumentException,Microsoft.PowerShell.Commands.NewWinEventCommand' |
| $thrownError = $_ | ||
| } | ||
|
|
||
| $thrownError.FullyQualifiedErrorId | Should Match $ExpectedFullyQualifiedErrorId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use ShouldBeErrorId like here:
PowerShell/test/powershell/Modules/Microsoft.PowerShell.Diagnostics/New-WinEvent.Tests.ps1
Line 27 in 0b7451b
| { New-WinEvent -ProviderName NonExistingProvider -Id 0 } | ShouldBeErrorId 'System.ArgumentException,Microsoft.PowerShell.Commands.NewWinEventCommand' |
| } | ||
|
|
||
| $result | Should BeOfType 'Microsoft.PowerShell.Commands.ByteCollection' | ||
| $result[0].ToString() | Should Match $ExpectedResult |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should use MatchExactly like here:
PowerShell/test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1
Line 499 in 7b8c6e7
| $ci[1].Name | Should MatchExactly $filenamePattern |
|
|
||
| if ($result.count -gt 1) | ||
| { | ||
| $result[1].ToString() | Should Match $ExpectedSecondResult |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should use MatchExactly like here:
PowerShell/test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1
Line 499 in 7b8c6e7
| $ci[1].Name | Should MatchExactly $filenamePattern |
|
|
||
| $result.count | Should Be $Count | ||
| $result | Should BeOfType 'Microsoft.PowerShell.Commands.ByteCollection' | ||
| $result[0].ToString() | Should Match $ExpectedResult |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should use MatchExactly like here:
PowerShell/test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1
Line 499 in 7b8c6e7
| $ci[1].Name | Should MatchExactly $filenamePattern |
| $errorThrown.FullyQualifiedErrorId | Should Match $ExpectedFullyQualifiedErrorId | ||
|
|
||
| $output.Length | Should Be 1 | ||
| $output[0].ToString() | Should Match $inputText1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should use MatchExactly like here:
PowerShell/test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1
Line 499 in 7b8c6e7
| $ci[1].Name | Should MatchExactly $filenamePattern |
| $result[1].ToString() | Should be "00000010 72 65 20 74 65 78 74 re text " | ||
| $result.Count | Should Be 3 | ||
| $result[0].ToString() | Should be "00000000 4E 6F 77 20 69 73 20 74 68 65 20 77 69 6E 74 65 Now is the winte" | ||
| $result[1].ToString() | Should be "00000010 72 20 6F 66 20 6F 75 72 20 64 69 73 63 6F 6E 74 r of our discont" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should use MatchExactly like here:
PowerShell/test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1
Line 499 in 7b8c6e7
| $ci[1].Name | Should MatchExactly $filenamePattern |
| $result[0].ToString() | Should be "00000000 54 68 69 73 20 69 73 20 61 20 62 69 74 20 6D 6F This is a bit mo" | ||
| $result[1].ToString() | Should be "00000010 72 65 20 74 65 78 74 re text " | ||
| $result.Count | Should Be 3 | ||
| $result[0].ToString() | Should be "00000000 4E 6F 77 20 69 73 20 74 68 65 20 77 69 6E 74 65 Now is the winte" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should use MatchExactly like here:
PowerShell/test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1
Line 499 in 7b8c6e7
| $ci[1].Name | Should MatchExactly $filenamePattern |
| $result.Count | Should Be 3 | ||
| $result[0].ToString() | Should be "00000000 4E 6F 77 20 69 73 20 74 68 65 20 77 69 6E 74 65 Now is the winte" | ||
| $result[1].ToString() | Should be "00000010 72 20 6F 66 20 6F 75 72 20 64 69 73 63 6F 6E 74 r of our discont" | ||
| $result[2].ToString() | Should be "00000020 65 6E 74 ent " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should use MatchExactly like here:
PowerShell/test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1
Line 499 in 7b8c6e7
| $ci[1].Name | Should MatchExactly $filenamePattern |
|
Thanks @adityapatwardhan for the comments - fixed all the Match to MatchExactly. |
adityapatwardhan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Addressing issue #3382 'Use -TestCases parameter in FormatHex.Tests.ps1'.