Improve TxPool tests and documentation #2327
Conversation
AurelienFT
changed the title
AurelienFT
requested review from
xgreenx,
Dentosal and
MitchTurner
as code owners
|
|
||
| #[cfg(test)] | ||
| /// Asserts the integrity of the collision manager. | ||
| fn assert_integrity(&self, expected_txs: &[ArcPoolTx]); |
There was a problem hiding this comment.
does this need to be in the trait?
You are forcing all implementors of a CollisionManager to implement a test-only function to establish its integrity.
There was a problem hiding this comment.
Pool is only defined with generics based on trait and so traits need to implements this for the pool to use it. Personally, I don't find this strange to have trait having test-only methods
There was a problem hiding this comment.
I prefer to keep traits only with business logic, if possible. In the case of tests, you always know the actual type. The TxPool can expose collision manager in the case of tests, in this case you can call assert_integrity directly on txpool.collision_manager.assert_integrity.
There was a problem hiding this comment.
Okey I personally find it cool to have test methods on trait also. But ok to change as you prefer.
Co-authored-by: Andrea Cerone <[email protected]>
Co-authored-by: Andrea Cerone <[email protected]>
|
|
||
| #[cfg(test)] | ||
| /// Asserts the integrity of the collision manager. | ||
| fn assert_integrity(&self, expected_txs: &[ArcPoolTx]); |
There was a problem hiding this comment.
I prefer to keep traits only with business logic, if possible. In the case of tests, you always know the actual type. The TxPool can expose collision manager in the case of tests, in this case you can call assert_integrity directly on txpool.collision_manager.assert_integrity.
| #![deny(clippy::arithmetic_side_effects)] | ||
| #![deny(clippy::cast_possible_truncation)] | ||
| #![deny(unused_crate_dependencies)] | ||
| #![deny(warnings)] | ||
|
|
||
| // TODO: Rename the folder from `txpool_v2` to `txpool` after the migration is complete. |
There was a problem hiding this comment.
We still need to rename txpool_v2 to txpool =D
There was a problem hiding this comment.
Dunno why but in my mind I saw you did it in a PR but it seems it was in my dreams 😆 . Readded
| pub fn assert_pool_integrity(&self, expected_txs: &[ArcPoolTx]) { | ||
| let pool = self.pool.as_ref().unwrap(); | ||
| let pool = pool.read(); | ||
| pool.assert_integrity(expected_txs); |
There was a problem hiding this comment.
It is the place were we can call assert_integrity for all internal fields of the TxPool.
Linked Issues/PRs
Closes #762
Description
This PR improves the tests to make sure all the structures that stores transactions/caches have the expected state after the execution of a test.
We also added an high level documentation for users and contributors at the root of the pool
Checklist
Before requesting review