Update format-hex tests to include -TestCase parameter - #3800
Conversation
There was a problem hiding this comment.
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.
|
Maria Romero (@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. |
|
James Truher (@JamesWTruher) or Francisco Gamino (@Francisco-Gamino) - Do you guys have any other concerns or comments? |
|
Maria Romero (@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.
We should check $output.Count >=1 or =1 not $null
There was a problem hiding this comment.
Changed.
There was a problem hiding this comment.
We can remove - next line make the check.
There was a problem hiding this comment.
Thanks, removed in all mentioned instances.
There was a problem hiding this comment.
We can remove - next line make the check.
There was a problem hiding this comment.
We can remove - next line make the check.
There was a problem hiding this comment.
We can remove - next line make the check.
There was a problem hiding this comment.
We can remove - next line make the check.
There was a problem hiding this comment.
We can remove - next line make the check.
8ef74e2 to
84a926b
Compare
|
James Truher (@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.
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.
Typo in Paramaters -> Parameters
| $output = Format-Hex -LiteralPath $InvalidPath, $inputFile1 -ErrorVariable errorThrown -ErrorAction SilentlyContinue | ||
| } | ||
|
|
||
| $errorThrown.FullyQualifiedErrorId | Should Match $ExpectedFullyQualifiedErrorId |
There was a problem hiding this comment.
Please use ShouldBeErrorId, like here:
| $thrownError = $_ | ||
| } | ||
|
|
||
| $thrownError.FullyQualifiedErrorId | Should Match $ExpectedFullyQualifiedErrorId |
There was a problem hiding this comment.
Use ShouldBeErrorId like here:
| } | ||
|
|
||
| $result | Should BeOfType 'Microsoft.PowerShell.Commands.ByteCollection' | ||
| $result[0].ToString() | Should Match $ExpectedResult |
There was a problem hiding this comment.
Should use MatchExactly like here:
|
|
||
| if ($result.count -gt 1) | ||
| { | ||
| $result[1].ToString() | Should Match $ExpectedSecondResult |
There was a problem hiding this comment.
Should use MatchExactly like here:
|
|
||
| $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.
Should use MatchExactly like here:
| $errorThrown.FullyQualifiedErrorId | Should Match $ExpectedFullyQualifiedErrorId | ||
|
|
||
| $output.Length | Should Be 1 | ||
| $output[0].ToString() | Should Match $inputText1 |
There was a problem hiding this comment.
Should use MatchExactly like here:
| $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.
Should use MatchExactly like here:
| $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.
Should use MatchExactly like here:
| $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.
Should use MatchExactly like here:
|
Thanks Aditya Patwardhan (@adityapatwardhan) for the comments - fixed all the Match to MatchExactly. |
Aditya Patwardhan (adityapatwardhan)
left a comment
There was a problem hiding this comment.
LGTM
* update tests to include -TestCase paramater in It block * Move test functions to appropriate context blocks * combine skipTest conditions * move skipTest to Describe block * correct typo, change to MatchExactly
Addressing issue #3382 'Use -TestCases parameter in FormatHex.Tests.ps1'.