Add Unit Tests to process_da_proposal()
#129
Conversation
dailinsubjam
changed the title
builder_state.rsprocess_da_proposal()
| // check the presence of quorum_proposal.data.view_number-1 in the spawned_builder_states list | ||
| if qc_msg.proposal.data.justify_qc.view_number != self.built_from_proposed_block.view_number |
There was a problem hiding this comment.
Hmm... You added this back in? I had removed this primarily due to handling similar logic with the am_i_the_best_builder_state_to_extend logic, and to make the process_quorum_proposal and process_da_proposal have similar logic paths.
I think with this present, some of the order tests don't pass.
There was a problem hiding this comment.
@Ayiga oops sorry I think this is because I didn't look carefully when doing conflict resolving, I shouldn't be changing anything in process_quorum_proposal(), let me recheck all these.
| .process_da_proposal(practice_da_msg_2.clone()) | ||
| .await; | ||
| } else { | ||
| tracing::error!("Not a da_proposal_message in correct format"); |
There was a problem hiding this comment.
Nit: I think it's better to panic directly in tests to make it explicit that we hit something unexpected.
There was a problem hiding this comment.
Looks like there are several tracing left in this file that could be replaced by panic.
There was a problem hiding this comment.
One more tracing updated, for other tracing they're in the implementation code rather than the test code so I just left them there.
Closes #134 and part of #133
This PR:
process_da_proposal().This PR does not:
builder_state.rsyet.Key places to review:
mod testinbuilder_state.rs.How to test this PR:
RUSTFLAGS="--cfg async_executor_impl=\"tokio\"" RUST_LOG=debug cargo test test_process_da_proposal