-
Notifications
You must be signed in to change notification settings - Fork 40
Fix integration test failures by making Node::with_conf more robust #213
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 integration test failures by making Node::with_conf more robust #213
Conversation
with_conf() is large, split out some of the variable creation sections into helper functions.
19069bf to
1571aec
Compare
tcharding
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.
Mad, good work man.
| } | ||
|
|
||
| if Self::wait_for_cookie_file(cookie_file.as_path(), Duration::from_secs(5)).is_err() { | ||
| let _ = process.kill(); |
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.
Maybe a code comment here saying why we kill the process if waiting for the cookie file fails?
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 have added comments to all the checks where it kills the process and retries.
node/src/lib.rs
Outdated
| } | ||
| thread::sleep(Duration::from_millis(200)); | ||
| } | ||
| unreachable!() |
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.
Perhaps this is cleaner?
for _ in 0..9 {
if let Ok(client) = Client::new_with_auth(rpc_url, auth.clone()) {
return Ok(client);
}
thread::sleep(Duration::from_millis(200));
}
Client::new_with_auth(rpc_url, auth.clone()).map_err(|e| Error::NoBitcoindInstance(e.to_string()).into())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.
Thanks, it is. I changed it and also rewrote create_client_wallet to use the same format.
|
NB. I think there is a CI issue, this passed CI but I got an error locally when I accidentally ran cargo test in the root instead of integration_test. The failure was a doctest on |
1571aec to
e0789b2
Compare
|
Made the suggested changes and added a few more comments. |
Spilt out client and wallet sections into helper functions with their own retry loops. Add wait functions for client and cookie file. Retry all of with_conf if it fails up to conf.attempts times. Improve the rustdocs.
This reverts commit c188d91.
e0789b2 to
8c9a4a4
Compare
tcharding
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.
ACK 8c9a4a4
…g Node::with_conf more robust
8c9a4a49e971782da87fbabdd0e7be318dded8b8 Revert "integration_test: Pause after node comes up" (Jamil Lambert, PhD)
66e7e7c3c188f09d27a7edede88c393cdf9318a5 Refactor Node::with_conf (Jamil Lambert, PhD)
2b8fad91fb885642f3d69268198374d92cd85ca0 Split out sections into helper functions (Jamil Lambert, PhD)
Pull request description:
The intermittent integration test failures are caused by race conditions in the creation of the node, wallet or the cookie file. Change `Node::with_conf` to check if there is a problem and retry the section where it occurred, or if that still fails retry the whole process of starting the node.
First patch:
- Split out some sections into helper functions.
Second patch:
- Remove the loop in `with_conf()` that creates `client`, instead loop over the individual parts.
- Create helper functions for creating the `client`, and creating or loading the client `wallet`, both with their own retry logic.
- Reduce the sleep time between retries to reduce the delay, since it looks like the errors happen over shorter time periods than the existing 1 second wait.
- Add functions to wait until the client and the cookie file are available.
- Add a loop to retry the whole process if it fails, since the integration tests pass when repeated, just repeat this section where the failures occur instead.
- Remove all the debugging outputs since it all works now they are not needed.
- Update the rustdocs for the function.
Third patch:
- Revert #206, since the issue is now solved at it’s source the pause is no longer needed.
Closes #205
ACKs for top commit:
tcharding:
ACK 8c9a4a49e971782da87fbabdd0e7be318dded8b8
Tree-SHA512: d80ef5e4def47025e830dd84aaa99e8ed0b71302f95bfc3d0ec210226444ce45a0679c569c5cbcf152cccc3222f812e4a8e5ab4ed5383a89ae2539f1f49baed4
The intermittent integration test failures are caused by race conditions in the creation of the node, wallet or the cookie file. Change
Node::with_confto check if there is a problem and retry the section where it occurred, or if that still fails retry the whole process of starting the node.First patch:
Second patch:
with_conf()that createsclient, instead loop over the individual parts.client, and creating or loading the clientwallet, both with their own retry logic.Third patch:
Closes #205