-
Notifications
You must be signed in to change notification settings - Fork 31
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
Test nodes leaving the network #1540
Conversation
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.
A few minor comments, otherwise looks good!
justfile
Outdated
@@ -30,8 +30,8 @@ _test_basic_tokio: | |||
echo Testing with async std executor | |||
RUST_LOG="" cargo test --features=tokio-ci --lib --bins --tests --benches --workspace --no-fail-fast test_basic -- --test-threads=1 --nocapture | |||
|
|||
test_with_failures: | |||
echo Testing with async std executor | |||
_test_with_failures: |
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.
Nit: Why do we need an underscore here? Wouldn't that imply we don't use this test? :)
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.
Was trying to use the same format as _test_basic
, but yeah I think we could remove the underscore! I'll remove it from _test_basic
as well.
tokio::test(flavor = "multi_thread", worker_threads = 2) | ||
)] | ||
#[cfg_attr(feature = "async-std-executor", async_std::test)] | ||
async fn test_with_failures_f() { |
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.
Nit: Maybe we could add a quick comment that these failures purposefully don't include DA nodes. I feel like we may forget this in the future when we switch to a rotating DA committee, and then we'll wonder why these tests fail. :)
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.
Oh, the failures actually include the DA nodes. The DA committee is the entire network for this test, so those 6 nodes to be killed are also DA nodes.
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.
Now failures only include DA nodes, so I added 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.
LGTM
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.
LGTM!
Adds tests for 1, f/2, and f nodes leaving the network.
Closes #1466.