[Test] Replace rpc_wallet_tests.cpp with python RPC unit tests #8450

Merged
merged 2 commits into from Aug 24, 2016

Projects

None yet

5 participants

@pstratem
Contributor
pstratem commented Aug 3, 2016

No description provided.

@sipa
Member
sipa commented Aug 4, 2016

Please elaborate why these tests are unnecessary.

@pstratem pstratem changed the title from Remove rpc_wallet_tests.cpp to [Test] Replace rpc_wallet_tests.cpp with python RPC unit tests Aug 4, 2016
@pstratem
Contributor
pstratem commented Aug 4, 2016

I've written python RPC tests to replace the tests.

It's much easier to maintain those tests and leads to fewer weird hacks in the setup for the tests (which is what I'm trying to avoid by removing the existing tests).

@jonasschnelli
Member

utACK on the additional RPC test, NACK on removing the unit test.

I don't think its wise to remove an already exiting unit test.
Also, UnitTests are on a different level then the RPC tests. Keep in mind: we ship the unit tests with our binaries.

@jonasschnelli jonasschnelli added the Tests label Aug 4, 2016
@pstratem
Contributor
pstratem commented Aug 4, 2016

@jonasschnelli it's an RPC unit test though...

@jonasschnelli
Member

I think we need to distinct between tests on code level (linked, boost unit test framework) and test from the outside (RPC tests). Both are useful.

@laanwj
Member
laanwj commented Aug 4, 2016 edited

ACK on removing the 'unit test'. I agree both kinds of tests are useful, however. Unit tests are supposed to test basic, low-level behavior.

E.g.:

  • Wallet unit tests test the CWallet object directly.
  • RPC unit tests tests basic RPC registration/dispatching.

The removed tests are clearly RPC (functional-level) tests disguised as unit tests. They are from the time that there wasn't a proper RPC testing framework in place yet, and are not good tests at that.

@pstratem
Contributor
pstratem commented Aug 5, 2016

@laanwj I've added tests for the addmultisigaddress RPC call

@MarcoFalke MarcoFalke commented on an outdated diff Aug 6, 2016
qa/rpc-tests/wallet-accounts.py
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import (
+ start_nodes,
+ start_node,
+ assert_equal,
+ connect_nodes_bi,
+)
+
+
+class WalletAccountsTest(BitcoinTestFramework):
+
+ def __init__(self):
+ super().__init__()
+ self.setup_clean_chain = True
+ self.num_nodes = 1
+ self.node_args = [[], []]
@MarcoFalke
MarcoFalke Aug 6, 2016 Member

Two nodes or one node?

@MarcoFalke MarcoFalke commented on an outdated diff Aug 6, 2016
qa/rpc-tests/wallet-accounts.py
+ super().__init__()
+ self.setup_clean_chain = True
+ self.num_nodes = 1
+ self.node_args = [[], []]
+
+ def setup_network(self):
+ self.nodes = start_nodes(self.num_nodes, self.options.tmpdir, self.node_args)
+ self.is_network_split = False
+
+ def run_test (self):
+ node = self.nodes[0]
+ # Check that there's no UTXO on any of the nodes
+ assert_equal(len(node.listunspent()), 0)
+
+ node.generate(101)
+ self.sync_all()
@MarcoFalke
MarcoFalke Aug 6, 2016 Member

You don't need sync when there is only one node.

@MarcoFalke
Member

@laanwj I've added tests for the addmultisigaddress RPC call

Can you check again. I couldn't find any new addmultisig rpc tests in the added files.

@MarcoFalke
Member

Concept ACK 20207c0

@MarcoFalke MarcoFalke commented on an outdated diff Aug 6, 2016
qa/rpc-tests/wallet-accounts.py
+ from_account = accounts[i]
+ to_account = accounts[(i+1)%len(accounts)]
+ to_address = account_addresses[to_account]
+ node.sendfrom(from_account, to_address, amount_to_send)
+
+ node.generate(1)
+
+ for account in accounts:
+ address = node.getaccountaddress(account)
+ assert(address != account_addresses[account])
+ assert_equal(node.getreceivedbyaccount(account), 2.0)
+ node.move(account, "", node.getbalance(account))
+
+ node.generate(101)
+
+ expected_account_balances = {"": 5200.00000000}
@MarcoFalke
MarcoFalke Aug 6, 2016 Member

Nit: No need for float

@pstratem
Contributor
pstratem commented Aug 8, 2016

@MarcoFalke nits picked

@jonasschnelli
Member

Agree with @laanwj.
utACK 9578333

@laanwj laanwj merged commit 9578333 into bitcoin:master Aug 24, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@laanwj laanwj added a commit that referenced this pull request Aug 24, 2016
@laanwj laanwj Merge #8450: [Test] Replace rpc_wallet_tests.cpp with python RPC unit…
… tests


9578333 Remove rpc_wallet_tests.cpp (Patrick Strateman)
25400c4 Account wallet feature RPC tests. (Patrick Strateman)
21857d2
@luke-jr luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Dec 21, 2016
@pstratem @luke-jr pstratem + luke-jr Account wallet feature RPC tests.
Github-Pull: #8450
Rebased-From: 25400c4
77b324e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment