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

pull pstratem wants to merge 2 commits into bitcoin:master from pstratem:2016-08-03-remove-rpc-wallet-tests changing 4 files +96 −231

pstratem commented at 11:47 PM on August 3, 2016: contributor

No description provided.
sipa commented at 12:37 AM on August 4, 2016: member

Please elaborate why these tests are unnecessary.
pstratem force-pushed on Aug 4, 2016
pstratem renamed this:
~~Remove rpc_wallet_tests.cpp~~
[Test] Replace rpc_wallet_tests.cpp with python RPC unit tests
on Aug 4, 2016
pstratem commented at 2:35 AM on August 4, 2016: contributor

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 commented at 5:50 AM on August 4, 2016: contributor

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 added the label Tests on Aug 4, 2016
pstratem commented at 6:36 AM on August 4, 2016: contributor

@jonasschnelli it's an RPC unit test though...
jonasschnelli commented at 6:39 AM on August 4, 2016: contributor

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 commented at 8:57 AM on August 4, 2016: member
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 commented at 12:45 AM on August 5, 2016: contributor

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

in qa/rpc-tests/wallet-accounts.py:None in 20207c0023 outdated

  16 | +
  17 | +    def __init__(self):
  18 | +        super().__init__()
  19 | +        self.setup_clean_chain = True
  20 | +        self.num_nodes = 1
  21 | +        self.node_args = [[], []]

MarcoFalke commented at 3:35 PM on August 6, 2016:

Two nodes or one node?

in qa/rpc-tests/wallet-accounts.py:None in 20207c0023 outdated

  28 | +        node = self.nodes[0]
  29 | +        # Check that there's no UTXO on any of the nodes
  30 | +        assert_equal(len(node.listunspent()), 0)
  31 | +        
  32 | +        node.generate(101)
  33 | +        self.sync_all()

MarcoFalke commented at 3:35 PM on August 6, 2016:

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

MarcoFalke commented at 3:38 PM on August 6, 2016: 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 commented at 3:39 PM on August 6, 2016: member

Concept ACK 20207c0

in qa/rpc-tests/wallet-accounts.py:None in 20207c0023 outdated

  63 | +            assert_equal(node.getreceivedbyaccount(account), 2.0)
  64 | +            node.move(account, "", node.getbalance(account))
  65 | +        
  66 | +        node.generate(101)
  67 | +        
  68 | +        expected_account_balances = {"": 5200.00000000}

MarcoFalke commented at 3:41 PM on August 6, 2016:

Nit: No need for float

pstratem force-pushed on Aug 7, 2016
pstratem force-pushed on Aug 7, 2016
Account wallet feature RPC tests. 25400c4de1
Remove rpc_wallet_tests.cpp 9578333ec4
pstratem force-pushed on Aug 8, 2016
pstratem commented at 2:30 AM on August 8, 2016: contributor

@MarcoFalke nits picked
jonasschnelli commented at 8:34 AM on August 24, 2016: contributor

Agree with @laanwj. utACK https://github.com/bitcoin/bitcoin/pull/8450/commits/9578333ec456bbfaccfecf9ea561c421f3a5d547
laanwj merged this on Aug 24, 2016
laanwj closed this on Aug 24, 2016
laanwj referenced this in commit 21857d2bf7 on Aug 24, 2016
luke-jr referenced this in commit 77b324ef1f on Dec 21, 2016
codablock referenced this in commit 6fb3221ea6 on Sep 19, 2017
codablock referenced this in commit 7b7b36b579 on Jan 9, 2018
codablock referenced this in commit e61556e20e on Jan 9, 2018
andvgal referenced this in commit f38c41b32e on Jan 6, 2019
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels