-
Notifications
You must be signed in to change notification settings - Fork 990
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
Attempt to kill bg process in tests #3894
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3894 +/- ##
=======================================
Coverage 73.24% 73.24%
=======================================
Files 341 341
Lines 105240 105240
=======================================
+ Hits 77085 77087 +2
+ Misses 28155 28153 -2 ☔ View full report in Codecov by Sentry. |
@@ -897,17 +910,22 @@ impl NamadaCmd { | |||
let mut cmd = self; | |||
loop { | |||
match abort_recv.try_recv() { |
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.
unrelated to this PR, but this try_recv
might be contributing substantially to the resource usage of e2e tests :P
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.
I guess the quick fix would be to call recv_timeout
instead with a sane timeout. Probably a better solution would be to switch to async channels?
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.
I think async channels wouldn't help. there is no event here triggering a new channel read, so the only reasonable thing would be what you suggested first: a poll with a timer
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.
Makes sense, I've opened #3897 to track that, I'll merge this one, thank you!
Attempt to kill bg process in tests (backport #3894)
Describe your changes
Some e2e tests rely on background processes, especially the IBC ones with
hermes
andgaiad
. There are cases in which the test fails and some of these tasks (especiallygaiad
) are not affected by the crash of the Namada node. In this case the runtime drops theNamadaBgCmd
but unfortunately dropping aJoinHandle
only detaches the target task. This is undesired cause if we run the test again the conflicting instances of the background process will cause another test failure.This PR attempts to run some cleanup code when dropping
NamadaBgCmd
to kill the task.Checklist before merging
breaking::
labelsnamada-docs
reponamada-indexer
ornamada-masp-indexer
, a corresponding PR is opened in that repo