node/approval-voting: Continue to migrate tests to subsystem tests #3471
Conversation
Lldenaurois
added
I5-tests
| @@ -394,7 +448,7 @@ fn test_harness<T: Future<Output = VirtualOverseer>>( | |||
| ApprovalVotingSubsystem::with_config( | |||
| Config{ | |||
| col_data: test_constants::TEST_CONFIG.col_data, | |||
| slot_duration_millis: 100u64, | |||
| slot_duration_millis: 5000u64, | |||
There was a problem hiding this comment.
Why did this need to change? Time should be totally programmatic in the tests. Also recommend extracting to a constant TEST_SLOT_DURATION or something.
| @@ -332,6 +333,60 @@ impl Backend for TestStore { | |||
| } | |||
| } | |||
|
|
|||
| #[derive(Clone)] | |||
| pub(super) struct SharedTestStore<T: Backend> { | |||
There was a problem hiding this comment.
I wonder if this needs to be generic. After all, in test code every type should be known. Or is this used with multiple different kinds of backends?
There was a problem hiding this comment.
If you feel this should still be a specific type, I can address that as well.
…for threshold tests
Lldenaurois
force-pushed
the
approval-voting-subsystem-tests
branch
from
eea91f9
to
34143d0
Compare
node/core/approval-voting/Cargo.toml
Outdated
| @@ -32,6 +32,7 @@ sp-application-crypto = { git = "https://github.com/paritytech/substrate", branc | |||
| sp-runtime = { git = "https://github.com/paritytech/substrate", branch = "master", default-features = false } | |||
|
|
|||
| [dev-dependencies] | |||
| async-std = "1.6.5" | |||
There was a problem hiding this comment.
seems it's only used for async_std::task::sleep which is an overkill, we can use futures_timer::Delay instead
There was a problem hiding this comment.
Ah yes, I forgot to remove async-std. Great catch, thank you!
| @@ -86,9 +86,6 @@ use crate::backend::{Backend, OverlayedBackend}; | |||
| #[cfg(test)] | |||
| mod tests; | |||
|
|
|||
| #[cfg(test)] | |||
| mod old_tests; | |||
| let (oracle, _handle) = make_sync_oracle(false); | ||
| let (mock_clock, assignment_criteria) = make_harness_input(); |
There was a problem hiding this comment.
nit: this is rather repetitive, a builder pattern with replacable defaults comes to mind that would reduce this a bit, but hey, it's a test, so feel free to ignore.
| @@ -616,11 +827,15 @@ fn ss_rejects_assignment_in_future() { | |||
|
|
|||
| // Add block hash 00. | |||
| ChainBuilder::new() | |||
| .add_block(block_hash, ChainBuilder::GENESIS_HASH, Slot::from(1), 1) | |||
| .add_block(block_hash, ChainBuilder::GENESIS_HASH, 1, BlockConfig { | |||
| assert!(clock.inner.lock().has_wakeup(slot_to_tick(slot) + tranche_offset)); | ||
| clock.inner.lock().wakeup_all(slot_to_tick(slot) + tranche_offset); | ||
|
|
||
| async_std::task::sleep(std::time::Duration::from_millis(200)).await; |
There was a problem hiding this comment.
I'd prefer to craft explicit dependecies ,i.e. a oneshot with a timeout to assure synchronization if possible.
| 13, // Alice wakeup | ||
| 20, // Check no shows |
| n_cores: validators.len() as _, | ||
| zeroth_delay_tranche_width: 5, | ||
| relay_vrf_modulo_samples: 3, | ||
| n_delay_tranches: 50, | ||
| no_show_slots: 2, |
There was a problem hiding this comment.
nit: A few comments on the intent why they were chosen as they would be nice
This PR addresses the current gap between tests.rs and old_tests.rs
The goal is to provide the same amount of coverage using full subsystem tests.