-
Notifications
You must be signed in to change notification settings - Fork 38.7k
test: add block height test to listsinceblock.py #17535
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: add block height test to listsinceblock.py #17535
Conversation
ariard
left a comment
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.
ACK 38e94bc modulo fixing only comment.
I think you can also harden wallet_reorgsrestore, wallet_listtransactions with block height assertions.
38e94bc to
da0fd46
Compare
jonatack
left a comment
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.
Thanks for the review @ariard. Took your suggestion.
promag
left a comment
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.
Concept ACK
practicalswift
left a comment
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.
Concept ACK
30eeb56 to
d289074
Compare
promag
left a comment
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.
ACK d28907423344a6bafb09ebd422a20cd11ff0cf41, I think 27450e0 could be squashed in ecbd8a2 or d289074.
and correct the header docstring.
d289074 to
5abe488
Compare
|
Thanks for reviewing, @promag! I took your suggestions and squashed the commit. |
@ariard thank you, I will look at those in a follow-up. @practicalswift, @MarcoFalke, any further suggestions here? |
5abe488 to
9501a27
Compare
|
Updated the release notes commit with @MarcoFalke's suggestion and rebased; no other changes. |
83e1d92 test: listsinceblock block height checks (Jon Atack) Pull request description: This is the second commit of #17535. This PR extends a listsinceblock test to check the new transaction 'blockheight' field recently added in #17437. It also cleans up code in the test function without changing or removing existing checks. ACKs for top commit: fjahr: tested ACK 83e1d92 ryanofsky: Code review ACK 83e1d92. Nice test improvements! Tree-SHA512: 92874b49a3bc0236500495f32dfcf683e1971ca3d4c51702c69ed4ce7dfce21273754f02f93d1243d73793701d9fdf49e14b149477cd249cbbd9e4e8d5bd49f8
83e1d92 test: listsinceblock block height checks (Jon Atack) Pull request description: This is the second commit of bitcoin#17535. This PR extends a listsinceblock test to check the new transaction 'blockheight' field recently added in bitcoin#17437. It also cleans up code in the test function without changing or removing existing checks. ACKs for top commit: fjahr: tested ACK 83e1d92 ryanofsky: Code review ACK 83e1d92. Nice test improvements! Tree-SHA512: 92874b49a3bc0236500495f32dfcf683e1971ca3d4c51702c69ed4ce7dfce21273754f02f93d1243d73793701d9fdf49e14b149477cd249cbbd9e4e8d5bd49f8
Follow-up to PR #17437.
Add block height test, expected fields tests, logging, and correct header docstring in
listsinceblock.py, and improve the release notes.