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

fix: call env.stop() on finish #2

Merged
merged 3 commits into from
Dec 16, 2022
Merged

fix: call env.stop() on finish #2

merged 3 commits into from
Dec 16, 2022

Conversation

waynexia
Copy link
Member

Signed-off-by: Ruihang Xia [email protected]

Changes Included

I forgot to call stop() on one env run finished. It may leave the started test target running and fail the next run. This patch fixes it.

src/runner.rs Outdated
@@ -124,6 +124,9 @@ impl<E: Environment> Runner<E> {
start.elapsed().as_millis()
);

// stop this env
self.env.stop(&env, db).await;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This stop won't be called if any error happens before this line. Maybe we should ensure that this stop will always be executed after L92 the start is called.

Copy link
Member Author

Choose a reason for hiding this comment

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

This stop won't be called if any error happens before this line. Maybe we should ensure that this stop will always be executed after L92 the start is called.

Makes sense. Maybe I should put it into a guardian.

Copy link
Member Author

Choose a reason for hiding this comment

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

I move start() and stop() out of run_env() in 1e456a9, which should prevent stop() from ignoring by early return. Please check it out!

src/runner.rs Outdated Show resolved Hide resolved
Signed-off-by: Ruihang Xia <[email protected]>
Copy link
Member

@jiacai2050 jiacai2050 left a comment

Choose a reason for hiding this comment

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

Will merge this since original issue has been resolved, thanks

@waynexia
Copy link
Member Author

Will merge this since original issue has been resolved, thanks

Much appreciated

@jiacai2050 jiacai2050 merged commit 3fe5d71 into main Dec 16, 2022
@waynexia waynexia deleted the env-stop branch December 16, 2022 07:54
@waynexia
Copy link
Member Author

Oops the merge method seems not to be "squash". Could you help to config it @jiacai2050? I don't have enough permission.

@jiacai2050
Copy link
Member

Done

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.

3 participants