Approval voting full subsystem tests#3391
Conversation
This commit introduces a Backend trait and attempts to move away from the Action model via an OverlayBackend as in the ChainSelection subsystem.
This commit modifies all tests to ensure tests are passing.
This commit addresses some oversights in the prior commit. 1. Inner errors in backend.write were swallowed 2. One-off write functions removed to avoid useless abstraction 3. Touch-ups in general
This commit removes the TestDB from tests.rs and replaces it with an in-memory kvdb.
Lldenaurois
force-pushed
the
approval-voting-full-subsystem-tests
branch
from
8f71209 to
ed06999
Compare
| @@ -1,3 +1,4 @@ | |||
| #![allow(dead_code)] | |||
There was a problem hiding this comment.
NOTE: Not all tests have been written and therefore some of the functions on the mock clock are considered dead_code. Pushing the PR up just to make sure the first steps are correct.
There was a problem hiding this comment.
is this going to be resolved in a follow up? what about old_tests?
|
|
||
| assert_eq!(rx.await, Ok(AssignmentCheckResult::Accepted)); | ||
|
|
||
| // TODO(ladi): fix |
There was a problem hiding this comment.
Having some trouble getting the wakeups scheduled. Needs a little more "thinking through" on my part.
drahnr
left a comment
drahnr
left a comment
There was a problem hiding this comment.
A few nits, mostly about repetitions and other minuscule things, other than that: LGTM
| DbBackend::new(db_writer.clone(), TEST_CONFIG) | ||
| } | ||
|
|
||
| fn overlay_txn<T, F>(db: &mut T, mut f: F) |
There was a problem hiding this comment.
yessir, this is from the previous PR. Should be more descriptive... great point.
| let state = State { | ||
| clock: Box::new(MockClock::new(tick)), | ||
| ..single_session_state(session_index, SessionInfo { | ||
| validators: validators.iter().map(|v| v.public().into()).collect(), |
There was a problem hiding this comment.
let validators = validators.iter() ... collect(); deserves an intermediate var.
| ), | ||
| }; | ||
|
|
||
| let tick = 9; |
There was a problem hiding this comment.
In this test are quite a few 🧙♂️ magic numbers, it would be nice to add a rationale why these are as they are.
| let block_hash = Hash::repeat_byte(0x01); | ||
| let maximum_broadcast = 10; | ||
|
|
||
| let candidate_entry: CandidateEntry = { | ||
| let approval_entry = approval_db::v1::ApprovalEntry { | ||
| tranches: Vec::new(), | ||
| backing_group: GroupIndex(0), | ||
| our_assignment: Some(approval_db::v1::OurAssignment { | ||
| cert: garbage_assignment_cert( | ||
| AssignmentCertKind::RelayVRFModulo { sample: 0 } | ||
| ), | ||
| tranche: maximum_broadcast, | ||
| validator_index: ValidatorIndex(4), | ||
| triggered: false, | ||
| }), | ||
| our_approval_sig: None, | ||
| assignments: bitvec::bitvec![BitOrderLsb0, u8; 0; 4], | ||
| approved: false, | ||
| }; | ||
|
|
||
| approval_db::v1::CandidateEntry { | ||
| candidate: Default::default(), | ||
| session: 1, | ||
| block_assignments: vec![(block_hash, approval_entry)].into_iter().collect(), |
There was a problem hiding this comment.
Seems to be rather repetitive and could be parameterize by a few variables - something worth to consider.
There was a problem hiding this comment.
Awesome feedback... exactly what I needed! Thank you!
Lldenaurois
added
B0-silent
coverage via full subsystem tests
Lldenaurois
force-pushed
the
approval-voting-full-subsystem-tests
branch
from
34490a7 to
0bacddb
Compare
…l-subsystem-tests
4 participants
This PR adds a first set of subsystem tests for the approval-voting subsystem. Not all paths are tested in this PR, meaning that the unit tests are still providing more coverage, but this gap will be bridged shortly.
Note: This PR is rebased on top of #3366, so we will need to merge that PR before this one can be merged. In the meantime, I will add tests to this PR in anticipation of the framework here not being too far from what is expected.