-
Notifications
You must be signed in to change notification settings - Fork 88
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(sequencer): add unit tests for src/api_state_ext #890
Conversation
// create inner rollup id/tx data | ||
let mut deposits = HashMap::new(); | ||
for _ in 0..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.
is this just adding 2 arbitrary deposit types?
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.
Yeah, I wanted to put rollup data in that could be read out. I can replace with normal non-deposit generated rollup transactions if you'd prefer.
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 is fine!
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.
looks good, main comment would be to use rand
instead of the manual number generator for generating the test data - you can probably use all zeros/constant values for the test data except for the fields you need to vary (height, deposit info, etc)
// create inner rollup id/tx data | ||
let mut deposits = HashMap::new(); | ||
for _ in 0..2 { | ||
let rollup_id = RollupId::new([rng.gen(); 32]); |
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.
just curious, can you do:
let rollup_id = RollupId::new([rng.gen(); 32]); | |
let rollup_id = RollupId::new(rng.gen()); |
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.
yes you can! changed
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.
looks good!
275eff3
to
77cc50b
Compare
Summary
Seventh set of state-ext unit tests. In total there are seven files which
need tests:
(This PR)astria-sequencer/src/api_state_ext.rs
astria-sequencer/src/state_ext.rs
astria-sequencer/src/accounts/state_ext.rs
astria-sequencer/src/asset/state_ext.rs
astria-sequencer/src/authority/state_ext.rs
astria-sequencer/src/bridge/state_ext.rs
astria-sequencer/src/ibc/state_ext.rs
This PR just tests the
src/api_state_ext/state_ext.rs
file.Background
It is good to have unit tests to ensure that the database works as intended.
Changes
Unit tests for the functionality in the file
src/api_state_ext/state_ext.rs
were added.Testing
:)