Skip to content

Conversation

@adityapatwardhan
Copy link
Member

  • About help test needed Update-Help
  • Tab completion tests were too stringent. There can be more than 1 matches.
  • Get-Process test should use $PID.
  • Improvements to Start-CodeCoverage script with respect to clean up and progress preference.
  • Removed code for uploading data to coveralls.
  • Added testexe path to $env:PATH

Copy link
Member

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Member

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

And casing parameters too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Member

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

And casing parameters too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

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.

Leave a comment

Copy link
Collaborator

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.

Copy link
Member Author

@adityapatwardhan adityapatwardhan Oct 9, 2017

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'.

Copy link
Collaborator

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.

Copy link
Member Author

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.

Copy link
Collaborator

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 ?

Copy link
Member Author

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Collaborator

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.

Copy link
Member Author

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Collaborator

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@adityapatwardhan
Copy link
Member Author

@SteveL-MSFT your feedback has been answered. Please have a look.

@daxian-dbw
Copy link
Member

Chatted with @adityapatwardhan offline and we found it strange that the Should complete about help topic test would fail, because we carry help content files in the package, and we pull it down from nuget package in build. So there shouldn't be a need to call update-help. @adityapatwardhan will investigate further to see what might be the root cause.

@adityapatwardhan
Copy link
Member Author

@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

@daxian-dbw
Copy link
Member

@adityapatwardhan Thanks for finding the root cause! It looks to me that that test shouldn't remove all about*.txt files, as it's testing module help contents and it shouldn't be a problem for about*.txt files to be present. Right?

@adityapatwardhan
Copy link
Member Author

@daxian-dbw your feedback has been answered. Please have a look.

Copy link
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

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

LGTM

@adityapatwardhan
Copy link
Member Author

@daxian-dbw I added another test fix. Please have a look.

@adityapatwardhan
Copy link
Member Author

@daxian-dbw All tests have passed. Please have a look.

Copy link
Member

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?

Copy link
Collaborator

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 

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Fixed.

@adityapatwardhan
Copy link
Member Author

@daxian-dbw I have updated the tests and all tests pass.

@daxian-dbw
Copy link
Member

@adityapatwardhan Can you please take a look at the test failures in Travis CI?

@adityapatwardhan
Copy link
Member Author

@daxian-dbw Thanks for the update.

@adityapatwardhan
Copy link
Member Author

@daxian-dbw All tests passed. Ready for merge.

@daxian-dbw daxian-dbw merged commit 486fb97 into PowerShell:master Oct 13, 2017
@adityapatwardhan adityapatwardhan deleted the testFixes branch October 13, 2017 22:32
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.

4 participants