-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Test fixes and code coverage automation fixes. #5046
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
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.
This is almost exactly the same as above, perhaps a helper function instead?
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.
Fixed.
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.
Since you're modifying this, might as well fix the casing Remove-Item
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.
And casing parameters too.
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.
Fixed.
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.
Search replace out-file with Out-File
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.
And casing parameters too.
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.
Fixed.
iSazonov
left a 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.
Leave a 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.
Maybe Should BeGreaterThan 0 - else next line can throw with useless message.
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.
@iSazonov Can you elaborate. I want to check it for being exactly 1. If the check fails, the next line will not be executed and the It will be marked as 'Failed'.
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 $res.CompletionMatches.Count = 0 then $res.CompletionMatches[0] will throw with generic message.
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.
But if $res.CompletionMatches.Count = 2 and we are using Should BeGreaterThan 0 then the test might pass but we are getting more matches than 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 wonder now - why are we removing this line with Should BeExactly 1 if we want just 1 ?
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.
Correct, I confused this with the about_help test. This can be BeGreaterThan 0.
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.
Fixed.
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.
Maybe Should BeGreaterThan 0 - else next line can throw with useless message.
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.
Correct, I confused this with the about_help test. This can be BeGreaterThan 0.
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.
Fixed.
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.
The same - maybe Should BeGreaterThan 0.
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.
Fixed.
|
@SteveL-MSFT your feedback has been answered. Please have a look. |
|
Chatted with @adityapatwardhan offline and we found it strange that the |
|
@daxian-dbw The updateable help system tests blow away the help and do not restore them. https://github.com/PowerShell/PowerShell/blob/master/test/powershell/engine/Help/UpdatableHelpSystem.Tests.ps1#L180 |
|
@adityapatwardhan Thanks for finding the root cause! It looks to me that that test shouldn't remove all |
|
@daxian-dbw your feedback has been answered. Please have a look. |
daxian-dbw
left a 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.
LGTM
|
@daxian-dbw I added another test fix. Please have a look. |
|
@daxian-dbw All tests have passed. Please have a look. |
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 will skip the tests on Unix plats. Is that intentional?
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.
Maybe
$skipTest = (Get-Command 'ssh' -ErrorAction SilentlyContinue) -eq $null
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.
Good point. Fixed.
|
@daxian-dbw I have updated the tests and all tests pass. |
|
@adityapatwardhan Can you please take a look at the test failures in Travis CI? |
* The updatable help system tests had not need to remove about help files. * Fixed tab completion tests to better disambiguate root/interop namespace. * Moved back the about_splatting test and removed update-help.
e548019 to
b5f23d9
Compare
|
@daxian-dbw Thanks for the update. |
|
@daxian-dbw All tests passed. Ready for merge. |
Update-Help