-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test(block_production): Add tests to block_production_task method #478
base: main
Are you sure you want to change the base?
Conversation
6b9d8ec
to
da627d1
Compare
4b05607
to
695e7de
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few nitpicks, otherwise all seems good.
1d2c093
to
3a371cf
Compare
@GMKrieger Could you please avoid force-pushing changes to a PR after a review has occurred? This makes it harder to review new changes since github cannot display "changes since last review". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good!
3a371cf
to
07e515d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @GMKrieger great job here!
In genreal I really like these test. I ask some changes. I general I would like to be more thorough on checking the state (empty, num txs) of:
- mempool
- pending block
- latest block
// keeps a consistent state | ||
backend | ||
.store_block( | ||
mp_block::MadaraMaybePendingBlock { | ||
info: mp_block::MadaraMaybePendingBlockInfo::Pending(mp_block::MadaraPendingBlockInfo { | ||
header: mp_block::header::PendingHeader::default(), | ||
tx_hashes: vec![Felt::ONE, Felt::TWO, Felt::THREE], | ||
}), | ||
inner: pending_inner.clone(), | ||
}, | ||
pending_state_diff.clone(), | ||
converted_classes.clone(), | ||
Some(visited_segments.clone()), | ||
Some(bouncer_weights), | ||
) | ||
.expect("Failed to store pending block"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happen with this PendingBlock
that we are storing? What should be the expected behavior.
Are we testing something that can even occur?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pending block is overwriting the previous pending block, which is not the one the task is working on. Now the database has an inconsistent state.
The BlockProductionTask struct has the block it's working on an variable called block
inside itself. This is the value we should work on and insert back on the database when finished.
This test currently tests a very edge case, let's say, someone tries to insert a pending block directly on the database and the block production task needs to keep working on the correct block.
But it's important to have a test like this because if there are future changes on the code that break this rule, this test will fail and warn the developer that something is off.
// have no transactions on it after the method ran for at | ||
// least block_time | ||
|
||
let result = block_production_task.on_block_time().await.expect_err("This should fail"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, what happens if this situation is reach?
Is there some recovery?
cc/ @Trantorian1 @GMKrieger
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not that I know of. It just fails and quits.
let pending_block: mp_block::MadaraMaybePendingBlock = backend.get_block(&DbBlockId::Pending).unwrap().unwrap(); | ||
|
||
assert!(mempool.is_empty()); | ||
assert!(pending_block.inner.transactions.is_empty()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check what happened with the tx?
|
||
assert!(mempool.is_empty()); | ||
assert!(pending_block.inner.transactions.is_empty()); | ||
assert_eq!(backend.get_latest_block_n().unwrap().unwrap(), 2); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be better to only add 1 tx just to have a difference within the previous one
Also check for the latest block number
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the extra tx, but I check the latest block number with assert_eq!(backend.get_latest_block_n().unwrap().unwrap(), 2);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job!
Pull Request type
Please add the labels corresponding to the type of changes your PR introduces:
What is the current behavior?
Resolves: #NA
The block production tests currently only test block sealing when starting a new service.
What is the new behavior?
This PR aims to add tests on the
block_production_task
method and related methods to thoroughly test the code.Does this introduce a breaking change?
No.
Other information
The testing format is based on #468. All feedback is welcome!