Skip to content

Conversation

@ryanofsky
Copy link
Contributor

Test only change.

New test covers importaddress, importpubkey, importprivkey, and importmulti RPC's.

This was originally part of #9137, but I split it off because the test is useful on its own.

Covers importaddress, importpubkey, importprivkey, and importmulti RPCs.
@fanquake fanquake added the Tests label Dec 13, 2016
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: no wildcard import here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

@ryanofsky ryanofsky force-pushed the pr/test-import-rescan branch from 6bbbe2c to 7e1d6fd Compare December 13, 2016 12:29
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 7e1d6fd43659e7cbfce2efd5433d79ef0fe687ec

(Sorry for sending nits your way a second time, feel free to not fix them; They are mostly not mandatory for the test to function.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from decimal import Decimal

would be cleaner

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't have to be too strict about not exceeding 80 chars. I think 120 is preferred, but there does not seem to be a strict rule, so meh.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I was just using yapf, but I increased the column width.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably does not change during the loop, so maybe cache it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assert that the transaction pool is empty, after each generate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@ryanofsky ryanofsky force-pushed the pr/test-import-rescan branch from 7e1d6fd to d8c0b9f Compare December 15, 2016 15:23
@laanwj
Copy link
Member

laanwj commented Dec 15, 2016

Thanks, LGTM
utACK d8c0b9f

@laanwj laanwj merged commit d8c0b9f into bitcoin:master Dec 15, 2016
laanwj added a commit that referenced this pull request Dec 15, 2016
d8c0b9f [qa] Add test for rescan feature of wallet key import RPCs (Russell Yanofsky)
codablock pushed a commit to codablock/dash that referenced this pull request Jan 18, 2018
…port RPCs

d8c0b9f [qa] Add test for rescan feature of wallet key import RPCs (Russell Yanofsky)
andvgal pushed a commit to energicryptocurrency/gen2-energi that referenced this pull request Jan 6, 2019
…port RPCs

d8c0b9f [qa] Add test for rescan feature of wallet key import RPCs (Russell Yanofsky)
CryptoCentric pushed a commit to absolute-community/absolute that referenced this pull request Feb 26, 2019
…port RPCs

d8c0b9f [qa] Add test for rescan feature of wallet key import RPCs (Russell Yanofsky)
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants