Skip to content
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

tests: kill process in interface-process-control as early as possible #13973

Merged

Conversation

ZeyadYasser
Copy link
Collaborator

Cleaning (killing) the spawned process was deferred to restore but spread waits for execute script to finish completely which causes the test to hang for the whole time of the process (sleep 5m).

If the test is successful we could speed up the process by popping the clean command.

This fix speeds up the test by 5 minutes.

Cleaning (killing) the spawned process was deferred to restore but spread
waits for execute script to finish completely which causes the test to hang
for the whole time of the process (sleep 5m).

If the test is successful we could speed up the process by popping the
clean command.

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>
@ZeyadYasser ZeyadYasser added Simple 😃 A small PR which can be reviewed quickly Test Robustness labels May 17, 2024
Copy link
Collaborator

@andrewphelpsj andrewphelpsj left a comment

Choose a reason for hiding this comment

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

Thank you! These minutes add up

Copy link
Contributor

@MiguelPires MiguelPires left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Copy link
Member

@olivercalder olivercalder left a comment

Choose a reason for hiding this comment

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

Really nice catch! I wanted to check whether it makes more sense to call kill directly at the end of the test, but I agree what you're doing with tests.cleanup pop is better. Thanks!

@ZeyadYasser
Copy link
Collaborator Author

Really nice catch! I wanted to check whether it makes more sense to call kill directly at the end of the test, but I agree what you're doing with tests.cleanup pop is better. Thanks!

@olivercalder the defer is necessary for the case when the test fails before the last kill command (#12611), and double kill could accidentally kill another process that reused the killed process pid.

@ZeyadYasser ZeyadYasser merged commit 3c541e7 into snapcore:master May 27, 2024
45 of 48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Simple 😃 A small PR which can be reviewed quickly Test Robustness
Projects
None yet
4 participants