-
Notifications
You must be signed in to change notification settings - Fork 452
Update Execute method for ProcessStartedHandler to prevent stalled tests #4447
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
Co-authored-by: aishwaryabh <37918412+aishwaryabh@users.noreply.github.com>
Co-authored-by: aishwaryabh <37918412+aishwaryabh@users.noreply.github.com>
Co-authored-by: aishwaryabh <37918412+aishwaryabh@users.noreply.github.com>
Co-authored-by: aishwaryabh <37918412+aishwaryabh@users.noreply.github.com>
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.
Pull Request Overview
This PR addresses a stalled test issue by introducing a timeout for the processStartedHandler and ensuring that processes are killed when the handler exceeds the allowed time. Key changes include:
- Adding a new E2E test to simulate a stalled process and verify that it is killed after a 2-minute timeout.
- Implementing a TimeoutHelper that wraps the processStartedHandler execution with a 2-minute timeout.
- Updating the Execute method to use the TimeoutHelper and forcibly kill the process if the handler does not complete in time.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
test/Cli/Func.E2E.Tests/Commands/FuncStart/TestsWithFixtures/NodeV4Tests.cs | Added a new E2E test to simulate and verify the timeout behavior for stalled processes. |
src/Cli/Abstractions/Helpers/TimeoutHelper.cs | Introduced a helper method to run tasks with a timeout. |
src/Cli/Abstractions/Command/Command.cs | Updated the Execute method to incorporate the timeout logic and handle process killing. |
Issue describing the changes in this PR
resolves #4392
After merging in #4364, when the tests are run locally today for core tools for func start, if the process starts up and is not interrupted within the test by the processStartedHandler (example: we expect the test to fail and throw an error before the host starts up, but it doesn't so the test is stuck since the host starts up successfully and is waiting for the user to manually kill the process), the test stalls, which is a flaw in the design.
To workaround this, a timeout would need to be added to the processTask here. If the timeout is reached before processStarted is finished, we have to manually kill the process by doing _process.Kill(true)
Pull request checklist