[tests] tidy up wallet_importmulti.py #15108
Conversation
Fixes review comments from PR 14886.
Adds a new wallet_util.py module and moves generic helper functions there: - get_key - get_multisig - test_address
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Concept ACK, will review later. |
promag
left a comment
promag
left a comment
There was a problem hiding this comment.
@amitiuttarwar side note, looks like you haven't added your email to github?
| @@ -0,0 +1,99 @@ | |||
| #!/usr/bin/env python3 | |||
| # Copyright (c) 2018 The Bitcoin Core developers | |||
There was a problem hiding this comment.
This was authored in 2018 (according to the git author date), so I guess it is fine.
|
utACK 2d5f1ea |
|
Concept ACK |
jnewbery
left a comment
jnewbery
left a comment
There was a problem hiding this comment.
Looks great. Thanks for picking this up @amitiuttarwar!
There were a couple of review comments from @practicalswift in #14952 which weren't addressed in the base PR #14565, so you could fix them in this PR if you wanted:
- wallet_importmulti.py L52 (
observed_warnings = result[0]['warnings']is underindented) - the default argument for
warningsintest_importmulti()should beNone, not[](see 'Mutable Default Arguments' in https://docs.python-guide.org/writing/gotchas/)
I've also added one small nit of my own.
tested ACK 2d5f1ea
| "timestamp": "now", | ||
| "keys": [wrong_privkey]}, | ||
| True, | ||
| success=True, |
There was a problem hiding this comment.
micronit: this line (and the following) is overindented. Remove one leading space (so success is aligned with the opening parens of the function call, not the the opening braces of the object above)
|
tACK 2d5f1ea Fixing nits is nice, but adding tests to #14491 will be easier once this is merged (cc @meshcollider). |
|
I am merging this, because it has some review and the nits are only about whitespace in touched lines or additional cleanup in unrelated lines. Please fix them up separately if anyone feels like it. |
2d5f1ea [tests] move wallet util functions to wallet_util.py (John Newbery) 6be64ef [tests] tidy up wallet_importmulti.py (John Newbery) Pull request description: Cherry picks un-merged commits from #14952, which "fixes review comments from @ryanofsky here: #14886 (review)" Tree-SHA512: 5f389196b0140d013a533d500f1812786a3a5cfb65980e13eaeacc459fddb55f43d05da3ab5e7cc8c997f26c0b667eed081ab6de2d125e631c70a7dd4c06e350
Summary: Fixes review comments from PR 14886. This is a partial backport of Core [[bitcoin/bitcoin#15108 | PR15108]] : bitcoin/bitcoin@6be64ef Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D6186
Summary: Adds a new wallet_util.py module and moves generic helper functions there: - get_key - get_multisig - test_address This is a partial backport of Core [[bitcoin/bitcoin#15108 | PR15108]] : bitcoin/bitcoin@2d5f1ea Depends on D6186 and D6185 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D6187
8 participants
Cherry picks un-merged commits from #14952, which "fixes review comments from @ryanofsky here: #14886 (review)"