-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Add a test for the powershell hang fix #5451
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
| exit | ||
| '@ | ||
| $process = Start-Process pwsh -ArgumentList $command -PassThru | ||
| for ($i = 0; -not $process.HasExited -and $i -lt 5; $i++) { |
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.
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.
OK, I will use Wait-UntilTrue
| Start-Sleep -Seconds 1 | ||
| } | ||
|
|
||
| $expect = "powershell process exits in 5 seconds" |
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.
Should the verification be like:
$process.HasExited | Should Be $true
Stop-Process $process -ErrorAction SilentlyContinueThere 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 Should be $true check will not generate a useful error message when it fails, so I prefer to the current check. But I will add -ErrorAction SilentlyContinue
|
@adityapatwardhan Your comments are addressed. Please take another look. Thanks! |
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
PaulHigin
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
Add test for the fix in PR #5356
Also add a minor change to build.psm1 to ignore the 'Sync-PSTags' warning.