8000 Add additional tests for Clear-Content to improve coverage by JamesWTruher · Pull Request #3157 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]

Skip to content

Add additional tests for Clear-Content to improve coverage #3157

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

JamesWTruher
Copy link
Collaborator

Coverage is reported to be more than 85% for Clear-Content

actual coverage for this is much higher (it looks like closing braces are not being counted as covered). I am following up with OpenCover folks.

@PowerShellTeam PowerShellTeam added the Review - Needed The PR is being reviewed label Feb 16, 2017
Copy link
Collaborator
@iSazonov iSazonov left a comment

Choose a reason for hiding this comment

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

We now have mixed capitalization for cmdlet names, parameter names, "should be". It is difficult to read. We should fix this (use "Should Be", "Set-Content"...) and parameters cuts.

}
finally {
pop-location
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

In our tests, we have too many deviations from our guide for throw "No Exception!"
I suggest using the excellent function ShouldBeErrorId
(I have already prepared new PR #3161 to move existing tests to this pattern)
The new look of this test:

  { 
      try {
        Push-Location function:
        Get-Command clear-content -stream $streamName
      }
      finally {
        Pop-Location
      }
  } | ShouldBeErrorId "NamedParameterNotFound,Microsoft.PowerShell.Commands.GetCommandCommand"

Only insert:

Import-Module $PSScriptRoot\..\..\Common\Test.Helpers.psm1

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have a bit of a problem with this. The functionality of checking the FullyQualifiedErrorId rather than the exception message should be in Pester rather than our environment. Putting this code in our own environment does not help the community, but only increases the burden on our code maintenance.

Copy link
Member

Choose a reason for hiding this comment

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

@JamesWTruher @iSazonov Let's create an issue for this discussion. I don't think this issue should block this PR. Please reply back with the issue and let's have any further discussion on the issue thread.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

issue created: #3187

@lzybkr lzybkr assigned lzybkr and TravisEz13 and unassigned lzybkr Feb 16, 2017
([char[]]($chars | get-random -count $length)) -join ""
}

function Get-NonExistantProviderName
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to have at least a small synopsis of the function

@TravisEz13
Copy link
Member

restarted Linux in TravisCI

@TravisEz13 TravisEz13 merged commit c6bd000 into PowerShell:master Mar 6, 2017
@iSazonov iSazonov removed the Review - Needed The PR is being reviewed label Mar 27, 2017
@JamesWTruher JamesWTruher deleted the jameswtruher/ClearContentCoverageTest branch May 11, 2022 16:35
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.

6 participants
0