-
Notifications
You must be signed in to change notification settings - Fork 26
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(starknet_integration_tests): test mempool syncing in e2e flow test #2628
Conversation
1e2aa1b
to
1671613
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2628 +/- ##
==========================================
- Coverage 40.10% 35.07% -5.04%
==========================================
Files 26 276 +250
Lines 1895 32163 +30268
Branches 1895 32163 +30268
==========================================
+ Hits 760 11281 +10521
- Misses 1100 19878 +18778
- Partials 35 1004 +969 ☔ View full report in Codecov by Sentry. |
1671613
to
eb9df0a
Compare
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.
Reviewed 1 of 4 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @alonh5 and @yair-starkware)
crates/starknet_integration_tests/src/flow_test_setup.rs
line 61 at r2 (raw file):
// order to make sure the ports are not overlapping. let sequencer_0_consensus_manager_config = consensus_manager_configs.remove(0); let sequencer_0_mempool_p2p_config = mempool_p2p_configs.remove(0);
Consider converting to array with try_into and use array unpacking
let [sequencer_0_mempool_p2p_config, sequencer_1_mempool_p2p_config]: [MempoolP2pConfig; 2] = mempool_p2p_configs.try_into().unwrap();
crates/starknet_integration_tests/src/integration_test_setup.rs
line 59 at r2 (raw file):
let (mut consensus_manager_configs, _consensus_proposals_channels) = create_consensus_manager_configs_and_channels(SEQUENCER_INDICES.len()); let mut mempool_p2p_configs =
What's happening here? Are all the configs but one dropped?
crates/starknet_integration_tests/tests/end_to_end_flow_test.rs
line 58 at r2 (raw file):
let sequencers = [&mock_running_system.sequencer_0, &mock_running_system.sequencer_1]; // We use only the first sequencer's gateway to test that the mempools are syncing.
Make it clear, either by comments or (more preferrably) by code, that this is the validator
76b3213
to
cd4cf96
Compare
eb9df0a
to
1bf7560
Compare
87511a3
to
6fed659
Compare
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.
Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @alonh5 and @ShahakShama)
crates/starknet_integration_tests/src/flow_test_setup.rs
line 61 at r2 (raw file):
Previously, ShahakShama wrote…
Consider converting to array with try_into and use array unpacking
let [sequencer_0_mempool_p2p_config, sequencer_1_mempool_p2p_config]: [MempoolP2pConfig; 2] = mempool_p2p_configs.try_into().unwrap();
Done
crates/starknet_integration_tests/src/integration_test_setup.rs
line 59 at r2 (raw file):
Previously, ShahakShama wrote…
What's happening here? Are all the configs but one dropped?
There's only 1 sequencer in the integration test for now
crates/starknet_integration_tests/tests/end_to_end_flow_test.rs
line 58 at r2 (raw file):
Previously, ShahakShama wrote…
Make it clear, either by comments or (more preferrably) by code, that this is the validator
The proposer/validator changes each height
6fed659
to
d24f635
Compare
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.
Reviewed 1 of 4 files at r4, 3 of 3 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @alonh5)
crates/starknet_integration_tests/src/integration_test_setup.rs
line 59 at r2 (raw file):
Previously, yair-starkware (Yair) wrote…
There's only 1 sequencer in the integration test for now
Do you want to add a TODO to add more?
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.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @alonh5)
crates/starknet_integration_tests/src/integration_test_setup.rs
line 59 at r2 (raw file):
Previously, ShahakShama wrote…
Do you want to add a TODO to add more?
Itay's team is working on it
No description provided.