-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Add Unblock-File tests #2954
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
Add Unblock-File tests #2954
Conversation
be42dbc to
4c5f928
Compare
| It "With -LiteralPath': no file exist" { | ||
| try { | ||
| Unblock-File -LiteralPath nofileexist.ttt -ErrorAction Stop | ||
| throw "No Exception!" |
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 would prefer this to be following so that the logs on failure have good error message.
throw "No Exception thrown when FileNotFound,Microsoft.PowerShell.Commands.UnblockFileCommand was expected"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 have two "cons" (for all comments).
I believe we should follow the documentation (throw "No Exception!" ) and preserve consistent in all the tests that currently use this docs template. If a template is bad, then we should change it and bring all the tests to the new template.
If Unblock-File test returns FileNotFound, then this will not help our understanding of the problem's root and why the exception throw. (We should mention a Zone.Identify stream and give a link to a MSDN article in the message.) I believe this is redundant, since in any case we will always start with the analysis of the test code and do more manual tests. So I prefer to see Expected : {True} But was : {False}. that simple means that a file is not unlocked.
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 am fine with this being kept as "No Exception"
But as of the function returning a $true or $false, I would prefer it to be like below as the error will give us the line number where the should is located :
function Test-UnblockFile {
try {
Get-Content -Path $testfilepath -Stream Zone.Identifier -ErrorAction Stop | Out-Null
throw "No Exception"
}
catch {
$_.FullyQualifiedErrorId | Should Be "GetContentReaderFileNotFoundError,Microsoft.PowerShell.Commands.GetContentCommand"
}
}
It "With '-Path': file exist" {
Unblock-File -Path $testfilepath
Test-UnblockFile
}
It "With '-LiteralPath': file exist" {
Unblock-File -LiteralPath $testfilepath
Test-UnblockFile
}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.
If we move Should to a function we get line number in the function, don't we? It seems this is not what we want.
And I am waiting #2973
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.
@adityapatwardhan Issue #2973 was closed. Please continue the review.
| It "With '-Path': no file exist" { | ||
| try { | ||
| Unblock-File -Path nofileexist.ttt -ErrorAction Stop | ||
| throw "No Exception!" |
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.
Same as the other 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.
I am fine with this being kept as "No Exception"
|
|
||
| It "With '-Path': file exist" { | ||
| Unblock-File -Path $testfilepath | ||
| Test-UnblockFile | Should Be $true |
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.
If this test fails, the error message would be Expected : {True} But was : {False}. This does not help in debugging the failure. I would prefer this to be like:
Unblock-File -Path $testfilepath
try {
Get-Content -Path $testfilepath -Stream Zone.Identifier -ErrorAction Stop | Out-Null
throw "No exception thrown when GetContentReaderFileNotFoundError,Microsoft.PowerShell.Commands.GetContentCommand was expected"|
}
catch {
$_.FullyQualifiedErrorId | Should Be "GetContentReaderFileNotFoundError,Microsoft.PowerShell.Commands.GetContentCommand"
}|
|
||
| It "With '-LiteralPath': file exist" { | ||
| Unblock-File -LiteralPath $testfilepath | ||
| Test-UnblockFile | Should Be $true |
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.
Same as above.
| It "With -LiteralPath': no file exist" { | ||
| try { | ||
| Unblock-File -LiteralPath nofileexist.ttt -ErrorAction Stop | ||
| throw "No Exception!" |
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 am fine with this being kept as "No Exception"
But as of the function returning a $true or $false, I would prefer it to be like below as the error will give us the line number where the should is located :
function Test-UnblockFile {
try {
Get-Content -Path $testfilepath -Stream Zone.Identifier -ErrorAction Stop | Out-Null
throw "No Exception"
}
catch {
$_.FullyQualifiedErrorId | Should Be "GetContentReaderFileNotFoundError,Microsoft.PowerShell.Commands.GetContentCommand"
}
}
It "With '-Path': file exist" {
Unblock-File -Path $testfilepath
Test-UnblockFile
}
It "With '-LiteralPath': file exist" {
Unblock-File -LiteralPath $testfilepath
Test-UnblockFile
}|
LGTM |
|
@adityapatwardhan Thanks for code review! |
I believe that using
Testcasesadds unnecessary complexity in these tests.