Skip to content

Conversation

@MiaRomero
Copy link
Member

Addressing issue #3382 'Use -TestCases parameter in FormatHex.Tests.ps1'.

  • Replaced the 'foreach' with the -TestCases parameter in the test helper functions.

Copy link
Collaborator

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.

@mirichmo mirichmo self-assigned this May 18, 2017
@MiaRomero MiaRomero assigned MiaRomero and unassigned mirichmo May 18, 2017
@lzybkr lzybkr assigned mirichmo and unassigned MiaRomero May 18, 2017
@lzybkr
Copy link
Contributor

lzybkr commented May 18, 2017

@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.

@lzybkr lzybkr removed their request for review May 18, 2017 19:46
@mirichmo
Copy link
Member

@JamesWTruher or @Francisco-Gamino - Do you guys have any other concerns or comments?

@mirichmo
Copy link
Member

@MiaRomero - We need to add you to the Microsoft organization. I just noticed that you are not a member yet.

Copy link
Collaborator

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed.

Copy link
Collaborator

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.

Copy link
Member Author

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.

Copy link
Collaborator

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.

Copy link
Collaborator

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.

Copy link
Collaborator

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.

Copy link
Collaborator

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.

Copy link
Collaborator

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.

@MiaRomero MiaRomero force-pushed the formatHex-test-fix branch from 8ef74e2 to 84a926b Compare June 6, 2017 19:06
@MiaRomero
Copy link
Member Author

@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)
Copy link
Collaborator

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" {
Copy link
Member

Choose a reason for hiding this comment

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

Typo in Paramaters -> Parameters

Copy link
Member Author

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
Copy link
Member

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:

{ New-WinEvent -ProviderName NonExistingProvider -Id 0 } | ShouldBeErrorId 'System.ArgumentException,Microsoft.PowerShell.Commands.NewWinEventCommand'

$thrownError = $_
}

$thrownError.FullyQualifiedErrorId | Should Match $ExpectedFullyQualifiedErrorId
Copy link
Member

Choose a reason for hiding this comment

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

Use ShouldBeErrorId like here:

{ 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
Copy link
Member

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:

$ci[1].Name | Should MatchExactly $filenamePattern


if ($result.count -gt 1)
{
$result[1].ToString() | Should Match $ExpectedSecondResult
Copy link
Member

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:

$ci[1].Name | Should MatchExactly $filenamePattern


$result.count | Should Be $Count
$result | Should BeOfType 'Microsoft.PowerShell.Commands.ByteCollection'
$result[0].ToString() | Should Match $ExpectedResult
Copy link
Member

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:

$ci[1].Name | Should MatchExactly $filenamePattern

$errorThrown.FullyQualifiedErrorId | Should Match $ExpectedFullyQualifiedErrorId

$output.Length | Should Be 1
$output[0].ToString() | Should Match $inputText1
Copy link
Member

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:

$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"
Copy link
Member

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:

$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"
Copy link
Member

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:

$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 "
Copy link
Member

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:

$ci[1].Name | Should MatchExactly $filenamePattern

@MiaRomero
Copy link
Member Author

Thanks @adityapatwardhan for the comments - fixed all the Match to MatchExactly.

Copy link
Member

@adityapatwardhan adityapatwardhan left a comment

Choose a reason for hiding this comment

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

LGTM

@mirichmo mirichmo merged commit ea77b57 into PowerShell:master Aug 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants