-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
Add additional tests for Clear-Content to improve coverage #3157
Conversation
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.
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 | ||
} |
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.
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
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 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.
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.
@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.
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.
issue created: #3187
([char[]]($chars | get-random -count $length)) -join "" | ||
} | ||
|
||
function Get-NonExistantProviderName |
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.
It would be nice to have at least a small synopsis of the function
Coverage has improved to more than 85% for Clear-Content
0b82783
to
fdf00b2
Compare
restarted Linux in TravisCI |
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.