feat: Update Relayer to use module level MockDb #576
Conversation
bvrooman
requested review from
ControlCplusControlV,
leviathanbeak and
Voxelot
bvrooman
changed the title
| blocks.handle_block_commit(b1, 2u64.into(), 2, IsReverted::False); | ||
| blocks.handle_block_commit(b2, 3u64.into(), 3, IsReverted::False); | ||
| blocks.handle_block_commit(b2, 3u64.into(), 3, IsReverted::True); | ||
|
|
||
| db.tap_sealed_blocks_mut(|sealed_blocks| { |
There was a problem hiding this comment.
would it make sense to have a helper test function that takes N amount of blocks and creates them instead of repating this process in multiple tests?
There was a problem hiding this comment.
Personally, I prefer having the test setup localized to the test itself, even if it means repeated code. Extra helper functions make setup less localized, and may obfuscate test setup (even if it's just by a little bit). The spirit of this PR is to be explicit in the test setup and to increase code locality (by removing the DummyDb and its implicit setup) and I think that such an approach would be counter to that.
I understand the suggestion, but personally I would like to avoid it if I can!
Edit: That isn't to say that helper functions do not have a place. I like to use them in ways that simplify setup without reducing locality. For example, I may use helper functions to create instances of test data, but I prefer to mutate the database within the test itself.
| where | ||
| F: Fn(&mut HashMap<BlockHeight, Arc<SealedFuelBlock>>) -> R, | ||
| { | ||
| func(&mut self.data.lock().unwrap().sealed_blocks) |
There was a problem hiding this comment.
I'd usually point too many unwraps but since this is a MockDb, and unwraps are only assuming the mutex is not poisoned it should be fine
There was a problem hiding this comment.
Yes, it's not the most "proper" approach to data access, but we can make that assumption in tests. I do not think MockDb will be a production-ready replacement for RocksDB anytime soon, unfortunately (fortunately?).
There was a problem hiding this comment.
Within the impl of the mock db itself, using lock()/unwrap() internally is fine. However, I think these tests will likely need to be re-written / redesigned since they are making assertions directly on the internals of the mockdb hashmaps instead of interacting with the relayerdb interface. I think it's fine to defer fixing these tests to a broader relayer cleanup initiative though.
Related issues:
This PR removes the Relayer tests' dependency on the existing
DummyDbthat comes pre-seeded, and uses a module level database,MockDbinstead.MockDbdoes not come seeded with any data, and tests are expected to perform explicit setup. This means tests become more descriptive, self-documenting, and stable by removing sensitivity to data changes across shared test configurations.The Relayer module is also the last area where the
DummyDbis used. Once this PR is merged, we can also remove theDummyDbentirely, allowing us to move closer towards a convention of explicit test setup.