Skip to content

Add listunspent() test for spendable/unspendable UTXO #7822

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Apr 19, 2016
Merged

Add listunspent() test for spendable/unspendable UTXO #7822

merged 2 commits into from
Apr 19, 2016

Conversation

joaopaulofonseca
Copy link
Contributor

@joaopaulofonseca joaopaulofonseca commented Apr 6, 2016 •
edited
Loading

This PR adds a test on wallet.py covering the behavior of spendable/unspendable value for UTXO, when calling RPC listunspent.

This value should be different for address or private key import.

@joaopaulofonseca joaopaulofonseca mentioned this pull request Apr 6, 2016
Closed
@laanwj laanwj added the Tests label Apr 6, 2016
@joaopaulofonseca joaopaulofonseca mentioned this pull request Apr 6, 2016
Closed
maflcko
maflcko reviewed Apr 6, 2016
View reviewed changes
qa/rpc-tests/listunspent.py
for txin in rawtx["vin"]:
for utxo in list_unspent_after:
if (txin["txid"] == utxo["txid"] and txin["vout"] == utxo["vout"]):
raise AssertionError("Fount used output on UTXOs list")
Copy link
Member

Choose a reason for hiding this comment

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

Nit: typo

@maflcko
Copy link
Member

maflcko commented Apr 6, 2016

Thanks, Concept ACK.

@joaopaulofonseca joaopaulofonseca force-pushed the support/add-test-listunspent branch from 0d8581d to 534ef0c Compare April 6, 2016 17:30
@joaopaulofonseca joaopaulofonseca changed the title [qa] Add test to RPC listunspent Add test to RPC listunspent Apr 7, 2016
@laanwj
Copy link
Member

laanwj commented Apr 14, 2016

I'm a bit divided whether I should propose to add this into the wallet test, which already uses listunspent. This currently adds some startup/teardown overhead (test takes ~9 seconds) just to test one call.
I don't feel strongly about that though.
What do you think @MarcoFalke?

@maflcko
Copy link
Member

maflcko commented Apr 15, 2016

startup/teardown overhead

I think such overhead is rather small. (Note that disablewallet.py runs in less than a second)

The overhead probably is caused by setting up the bidirectional connections and sync_all() calls.

I have not yet looked closely at the test (I try to avoid large diffs right now in the /qa subfolder as each patch may or may not silently conflict with the python3 refactoring work.)

@jpdffonseca Would you mind to take a look if this can be neatly squashed into the wallet.py test, as suggested by @laanwj ?

@joaopaulofonseca
Copy link
Contributor Author

@MarcoFalke and @laanwj yes that makes sense. I will try to squash this verifications into wallet.py.

@joaopaulofonseca joaopaulofonseca force-pushed the support/add-test-listunspent branch 2 times, most recently from 0f714f0 to 3b2eedc Compare April 18, 2016 13:52
@joaopaulofonseca joaopaulofonseca changed the title Add test to RPC listunspent Add wallet test to check spendable/unspendable UTXO on RPC listunspent Apr 18, 2016
@joaopaulofonseca
Copy link
Contributor Author

joaopaulofonseca commented Apr 18, 2016 •
edited
Loading

@MarcoFalke, after checking that wallet.py already covers some of the tests for listuspent(), I discarded the separate test file and merged the tests regarding the import of bitcoin addresses and private keys into wallet.py. The test execution time should remain the same.

@joaopaulofonseca joaopaulofonseca changed the title Add wallet test to check spendable/unspendable UTXO on RPC listunspent Add listunspent test for spendable/unspendable UTXO Apr 18, 2016
@joaopaulofonseca joaopaulofonseca changed the title Add listunspent test for spendable/unspendable UTXO Add listunspent() test for spendable/unspendable UTXO Apr 18, 2016
@maflcko
Copy link
Member

maflcko commented Apr 18, 2016

Thanks. Looks better now.

utACK 3b2eedc

maflcko
maflcko reviewed Apr 18, 2016
View reviewed changes
qa/rpc-tests/wallet.py
@@ -6,6 +6,27 @@
from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import *

def check_array_result(object_array, to_match, expected):
Copy link
Member

Choose a reason for hiding this comment

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

If you copied this function from elsewhere, wouldn't it make sense to move it instead to test_framework/util.py, so the utility function can be used in other tests as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed.. It is used by multiple tests. I think it comes in handy for current and future test cases. If you guys agree I guess this could be moved to test_framework/util.py to be reused whenever needed..

Copy link
Member

Choose a reason for hiding this comment

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

Sure, go ahead.

Copy link
Member

Choose a reason for hiding this comment

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

Do you intend to do so in this pull? If so I'll wait with merging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, since we are on this topic.

@joaopaulofonseca joaopaulofonseca force-pushed the support/add-test-listunspent branch from 3b2eedc to 5d217de Compare April 19, 2016 11:29
@joaopaulofonseca
Copy link
Contributor Author

Moved assert_array_result() to test_framework/util.py.

I noticed that the previous method on receivedby.py had a bug when using the flag should_not_find.
If the flag is set to True, the validation wouldn't work correctly when the values passed as to_match are indeed found inside object_array, and no assertion is raised.

@laanwj
Copy link
Member

laanwj commented Apr 19, 2016

utACK 5d217de

@laanwj laanwj merged commit 5d217de into bitcoin:master Apr 19, 2016
laanwj added a commit that referenced this pull request Apr 19, 2016
@laanwj
Merge #7822: Add listunspent() test for spendable/unspendable UTXO
0e6fd5e 
5d217de Add test to check spendable and unspendable UTXO on RPC listunspent (Joao Fonseca)
fa942c7 Move method to check matches within arrays on util.py (Joao Fonseca)
@fixe fixe deleted the support/add-test-listunspent branch April 19, 2016 13:06
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Apr 19, 2016
@joaopaulofonseca
Add listunspent() test for spendable/unspendable UTXO
6862627 
Github-Pull: bitcoin#7822
Rebased-From: fa942c7 5d217de
@dagurval dagurval mentioned this pull request Sep 2, 2017
Merged
@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.
Reviewers
No reviews
Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@joaopaulofonseca @maflcko @laanwj