-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Add more tab completion tests #4560
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 more tab completion tests #4560
Conversation
adityapatwardhan
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 with minor comment.
|
|
||
| AfterAll { | ||
| Pop-Location | ||
| Remove-Item -Path $tempDir -Recurse -Force |
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.
Please add -ErrorAction SilentlyContinue
| CommandLine = "Test history completion" | ||
| ExecutionStatus = "Stopped" | ||
| StartExecutionTime = "8/11/2017 11:04:10 AM" | ||
| EndExecutionTime = "8/11/2017 11:05:49 AM" |
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 dates is not explicitly used but it is culture dependent. Should our tests work under different cultures?
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.
@daxian-dbw Can you address feedback from @iSazonov
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 catch. Will change to use a DataTime object instead.
|
LGTM. |
The second round of adding tab completion tests - issue: #4160
30 tests are added to improve the test coverage in
CompletionAnalysis.csCompletionAnalysis.cs