Skip to content

Conversation

@iSazonov
Copy link
Collaborator

@iSazonov iSazonov commented Jan 5, 2017

I believe that using Testcases adds unnecessary complexity in these tests.

@PowerShellTeam PowerShellTeam added the Review - Needed The PR is being reviewed label Jan 5, 2017
It "With -LiteralPath': no file exist" {
try {
Unblock-File -LiteralPath nofileexist.ttt -ErrorAction Stop
throw "No Exception!"
Copy link
Member

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"

Copy link
Collaborator Author

@iSazonov iSazonov Jan 6, 2017

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.

Copy link
Member

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 
    }

Copy link
Collaborator Author

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

Copy link
Collaborator Author

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

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.

Copy link
Member

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

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

Choose a reason for hiding this comment

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

Same as above.

@mirichmo mirichmo added Review - Waiting on Author and removed Review - Needed The PR is being reviewed labels Jan 6, 2017
@PowerShellTeam PowerShellTeam added Review - Needed The PR is being reviewed and removed Review - Waiting on Author labels Jan 6, 2017
It "With -LiteralPath': no file exist" {
try {
Unblock-File -LiteralPath nofileexist.ttt -ErrorAction Stop
throw "No Exception!"
Copy link
Member

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 
    }

@adityapatwardhan
Copy link
Member

LGTM

@mirichmo mirichmo merged commit f0a9581 into PowerShell:master Jan 18, 2017
@iSazonov
Copy link
Collaborator Author

@adityapatwardhan Thanks for code review!

@iSazonov iSazonov deleted the unblockfiletests branch March 13, 2017 06:33
@iSazonov iSazonov removed the Review - Needed The PR is being reviewed label Mar 27, 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.

5 participants