-
Notifications
You must be signed in to change notification settings - Fork 136
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
test: post-shanghai block for eth_getBlockByHash #966
Conversation
tests/rpc_server.rs
Outdated
let value: Value = serde_yaml::from_str(&file).unwrap(); | ||
let all_blocks = value.as_sequence().unwrap(); | ||
let post_shanghai = all_blocks.last().unwrap(); | ||
assert_eq!(post_shanghai["number"], 1751000); |
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.
The point of this assertion right now is that if someone appends a new block into the yaml file, then this function would start testing a different block. So this assertion is a way of adding some confidence that we're testing the block that we think we are.
It probably makes sense for me to add that comment inline into the code above.
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.
Now that we're using yaml and comments are supported, seems like it might also be worth a trailing comment at the bottom of the yaml file
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.
I understand why we're adding the post-shanghai block, but don't understand why we're removing the test for the original (1000001
) block? Why aren't we leaving that test case in?
tests/rpc_server.rs
Outdated
let value: Value = serde_yaml::from_str(&file).unwrap(); | ||
let all_blocks = value.as_sequence().unwrap(); | ||
let post_shanghai = all_blocks.last().unwrap(); | ||
assert_eq!(post_shanghai["number"], 1751000); |
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.
Now that we're using yaml and comments are supported, seems like it might also be worth a trailing comment at the bottom of the yaml file
The previous asset file doesn't have the block body for that block number. Without the body, the test will fail, because it can't retrieve the transactions. Note that this PR change only affects the It should be straightforward to port over more of the older block assets from the same place I got this one (portal-hive), when support for older forks is a priority. |
fe96596
to
b6e0e26
Compare
Importantly, use a header that we have a block body for.
b6e0e26
to
474cf7f
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.
Importantly, use a header that we have a block body for.
What was wrong?
I didn't have easy access to the block body value for the header I was using for the json getBlock test. Also, our Devconnect target is for post-Shanghai, so that seems the more important era to test right now.
How was it fixed?
Pulled in (plus restructured) part of this file from portal-hive:
simulators/portal-interop/src/test-data/test_data_collection_of_forks_blocks.yaml
Split from #963
To-Do