-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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 round manager tests #15369
Fix round manager tests #15369
Conversation
⏱️ 17m total CI duration on this PR
|
seems some tests failing? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Which tests are failing? All forge tests seem to pass? |
consensus/src/round_manager_test.rs
Outdated
@@ -317,7 +324,7 @@ impl NodeSetup { | |||
let (round_manager_tx, _) = aptos_channel::new(QueueStyle::LIFO, 1, None); | |||
|
|||
let mut local_config = local_consensus_config.clone(); | |||
local_config.enable_broadcast_vote(false); | |||
local_config.enable_broadcast_vote(true); |
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.
Isn't this the default? We should remove this if so. That way test can fail if someone tried to change the config.
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.
Yes. The default is true. Removed this statement.
}, | ||
_ => panic!("unexpected network message {:?}", next_message), | ||
} | ||
// let next_message = timed_block_on(&runtime, nodes[proposal_node].next_network_message()); |
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.
Why is this commented out? If this test is incomplete, can we add a unimplemented!
here instead so it can break when someone tries to run it.
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.
Yeah. Actually, the test is flaky and there is already #[ignore]
tag added on top of the test. The test doesn't affect forge.
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 meant if someone tries to remove the #[ignore]
runs and thinks that the test passes because parts of code is commented out.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
✅ Forge suite
|
Description
The round manager tests are outdated. They work only when
broadcast_vote
andorder_vote
flags are disabled, which doesn't actually match with the mainnet settings. This PR aims to address the issue by fixing the round manager tests.How Has This Been Tested?
Key Areas to Review
Type of Change
Which Components or Systems Does This Change Impact?
Checklist