feat(tests): Add tests for Checkpoint and CheckpointExt #36
Conversation
varun-doshi
requested review from
SozinM,
julio4 and
karim-agha
as code owners
There was a problem hiding this comment.
Thank you for your contribution!
This is a good start, but I have some recommendations:
Here we want to unit tests a sensible component used everywhere, so try to add tests focused on specific methods in these modules. Take the time to think of all possible inputs/outputs and find as much edge cases as possible. You don't have to tests all methods of this interface at once, but that for each one you try to test you cover it well.
| #[test] | ||
| fn test_contains_is_false_without_txs() { | ||
| let (block, _) = BlockContext::<Ethereum>::mocked(); | ||
| let cp = block.start(); | ||
|
|
||
| let fake_hash = TxHash::repeat_byte(0x42); | ||
| assert!(!cp.contains(fake_hash)); | ||
| } |
There was a problem hiding this comment.
Can you test that it returns true on the new checkpoint obtained after applying this tx as well
| #[test] | ||
| fn test_is_empty_and_root() { | ||
| let (block, _) = BlockContext::<Ethereum>::mocked(); | ||
| let root = block.start(); | ||
| let mid = root.barrier(); | ||
| let leaf = mid.barrier(); | ||
|
|
||
| assert!(root.is_empty()); | ||
| assert_eq!(leaf.root(), root); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_gas_used_and_cumulative_gas_used() { | ||
| let (block, _) = BlockContext::<Ethereum>::mocked(); | ||
| let cp = block.start(); | ||
| assert_eq!(cp.gas_used(), 0); | ||
| assert_eq!(cp.cumulative_gas_used(), 0); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_effective_tip_and_blob_gas() { | ||
| let (block, _) = BlockContext::<Ethereum>::mocked(); | ||
| let cp = block.start(); | ||
| assert_eq!(cp.effective_tip_per_gas(), 0); | ||
| assert!(!cp.has_blobs()); | ||
| assert_eq!(cp.blob_gas_used(), Some(0)); | ||
| assert_eq!(cp.cumulative_blob_gas_used(), 0); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_to_between_linear_history() { | ||
| let (block, _) = BlockContext::<Ethereum>::mocked(); | ||
| let a = block.start(); | ||
| let b = a.barrier(); | ||
| let c = b.barrier(); | ||
|
|
||
| let span1 = c.to(&a).unwrap(); | ||
| let span2 = a.to(&c).unwrap(); | ||
|
|
||
| assert_eq!(span1.len(), 3); | ||
| assert_eq!(span2.len(), 3); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_balance_and_nonce_defaults() { | ||
| let (block, _) = BlockContext::<Ethereum>::mocked(); | ||
| let cp = block.start(); | ||
|
|
||
| let addr = Address::ZERO; | ||
| assert_eq!(cp.balance_of(addr).unwrap(), U256::ZERO); | ||
| assert_eq!(cp.nonce_of(addr).unwrap(), 0); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_signers_and_nonces_are_empty() { | ||
| let (block, _) = BlockContext::<Ethereum>::mocked(); | ||
| let cp = block.start(); | ||
|
|
||
| assert!(cp.signers().is_empty()); | ||
| assert!(cp.nonces().is_empty()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_hash_and_is_bundle_and_has_failures_defaults() { | ||
| let (block, _) = BlockContext::<Ethereum>::mocked(); | ||
| let cp = block.start(); | ||
|
|
||
| assert_eq!(cp.hash(), None); | ||
| assert!(!cp.is_bundle()); |
There was a problem hiding this comment.
For all the default values for the initial checkpoint, can you move all the checks in a single test test_new_at_block.
| let span1 = c.to(&a).unwrap(); | ||
| let span2 = a.to(&c).unwrap(); | ||
|
|
||
| assert_eq!(span1.len(), 3); | ||
| assert_eq!(span2.len(), 3); |
There was a problem hiding this comment.
span1 and span2 are equivalent.
Could you iterate over history and make sure it is the correct span.
to is quite complex so there are multiple tests to do:
- non linear error
- correct history when traversing back and forth
- to(x, y) equivalence with to(y, x)
and try to find edge cases in general
| #[test] | ||
| fn test_named_barrier_and_prev_depth() { | ||
| // Outline: | ||
| // 1. create initial checkpoint (depth 0) | ||
| // 2. create named barrier on top | ||
| // 3. verify new depth is 1, prev is initial, and is_named_barrier returns | ||
| // true | ||
| let root = { | ||
| let (block, _) = BlockContext::<Ethereum>::mocked(); | ||
|
|
||
| let cp = block.start(); | ||
| cp | ||
| }; | ||
|
|
||
| let named = root.named_barrier("sequencer-synced"); | ||
| assert_eq!(named.depth(), root.depth() + 1); | ||
| assert!(named.is_named_barrier("sequencer-synced")); | ||
| assert!(matches!(named.prev(), Some(_))); | ||
| assert_eq!(named.prev().unwrap().depth(), root.depth()); | ||
| } |
There was a problem hiding this comment.
remove this for now. We're reworking named barrier in #35
|
|
||
| let checkpoint = block.start(); | ||
|
|
||
| let now = Instant::now(); | ||
| assert!(checkpoint.created_at() <= now); |
There was a problem hiding this comment.
Try to be more precise, i.e. like:
let before = Instant::now();
let cp = block.start();
let after = Instant::now();
assert!((before..=after).contains(&cp.created_at()));| let checkpoint2 = checkpoint.barrier(); | ||
| let checkpoint3 = checkpoint2.barrier(); | ||
|
|
||
| let history: Vec<_> = checkpoint3.into_iter().collect(); | ||
| assert_eq!(history.len(), 3); | ||
| assert_eq!(history[0].depth(), 2); | ||
| assert_eq!(history[1].depth(), 1); | ||
| assert_eq!(history[2].depth(), 0); |
There was a problem hiding this comment.
Can you also add a checkpoint with an executable (tx/bundle).
And not only test depth but also that the yielded items are the ones expected.
| #[test] | ||
| fn test_barrier_depth_and_is_barrier() { | ||
| let (block, _) = BlockContext::<Ethereum>::mocked(); | ||
|
|
||
| let checkpoint = block.start(); | ||
|
|
||
| // Expected: initial checkpoint is depth 0 and is barrier | ||
| assert_eq!(checkpoint.depth(), 0); | ||
| assert!(checkpoint.is_barrier()); | ||
| assert!(checkpoint.prev().is_none()); | ||
| } |
There was a problem hiding this comment.
Use new_at_block instead of BlockContext::start
2 participants
Fixes #28
Will add more tests as advised