Skip to content
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.

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

[tests] Demostration/concept: Add a TestNode class to test_Framework.py #10082

Closed
wants to merge 11 commits into from

Conversation

Projects
None yet
4 participants
@jnewbery
Copy link
Member

commented Mar 26, 2017

This PR is too large for review and will need continually rebasing. This PR tracks the individual tasks to introduce a TestNode class. The individual commits are in separate PRs

Tasks:

  • #10171 Add initialize chain / start node / stop node methods to TestFramework. Initially these are wrapper methods which call into the functions in util.py (not disruptive - only touches test_framework files)
  • #10359 Update individual test cases to call TestFramework.start_node() etc (disruptive - touches all test cases, so will require rebases for open PRs)
  • #10556 Change TestFramework start/stop methods to contain the logic and remove the functions from util.py. Also move the stateful port and coverage functions into TestFramework (not disruptive - only touches test_framework files)
  • #10711 Add the TestNode class (not disruptive - only touches test_framework files)
  • #11182 Add p2p connections into the TestNode class (not disruptive - only touches test_framework files)
  • #11182 Update individual test cases to use TestNode class (marginally disruptive - does not need to be done all at once)

This PR adds a TestNode class to the test_framework.py module. The goals for this are:

  1. Present a clean interface to the test writer

The test writer should be able to set up a desired network topology and not have to think too hard about how to start bitcoind instances and setup RPC/P2P connections to them.

To create a node with an RPC and a P2P connection, tests currently look something like this:

        self.nodes = []
        self.nodes.append(start_node(0, self.options.tmpdir))
        node0 = SingleNodeConnCB()
        connections = []
        connections.append(NodeConn('127.0.0.1', p2p_port(0), self.nodes[0], node0))
        node0.add_connection(connections[0])
        NetworkThread().start()  # Start up network handling in another thread
        node0.wait_for_verack()

After this PR, that should be more like this:

        self.num_nodes = 1
        self.start_nodes()
        self.nodes[0].add_p2p_connection()

the tester now has a TestNode instance self.nodes[0] which he/she can use to send RPC commands, send P2P messages and register callbacks for received P2P messages.

  1. Remove global state from utils.py

There are currently several global state variables in utils.py (COVERAGE_DIR, PortSeed and bitcoind_processes). Having global state variables in the utils module is bad practice and makes importing from utils quite brittle. It also makes it difficult to add functionality to the test framework without inadvertently breaking something.

@fanquake fanquake added the Tests label Mar 26, 2017

@NicolasDorier

This comment has been minimized.

Copy link
Member

commented Mar 26, 2017

Very nice, concept ACK.

jnewbery added some commits Mar 23, 2017

Move global state to BitcoinTestFramework
This commit moves functions st{art,op}_nodes{,s}() functions
and bitcoind_processes and coverage variables into BitcoinTestFramework
Add default values to start_node() and start_nodes()
This commit adds default values to the num_nodes and dirname arguments
in start_node() and start_nodes(). All calls to these methods used
self.options.tmpdir and most used self.num_nodes for these values.
Having those as the defaults simplifies the interface.

This commit also changes sequential start_node() calls to a single
start_nodes() call where possible. When start_nodes() is updated to
start nodes in parallel this will make test startup faster.
Add TestNode class
This commit adds a TestNode class to test_framework.py. This class is
responsible for all state related to a bitcoind node under test. It
stores local state, is responsible for tracking the bitcoind process and
delegates unrecognised messages to the RPC connection.

@jnewbery jnewbery changed the title [tests] Add a TestNode class to test_Framework.py [tests] Demostration/concept: Add a TestNode class to test_Framework.py Mar 30, 2017

@jnewbery jnewbery force-pushed the jnewbery:refactor_utils_py branch to 328763a Apr 8, 2017

MarcoFalke added a commit that referenced this pull request Aug 15, 2017

Merge #10711: [tests] Introduce TestNode
7897338 [tests] Introduce TestNode (John Newbery)

Pull request description:

  Continues #10082

  TestNode is a class responsible for all state related to a bitcoind node
  under test. It stores local state, is responsible for tracking the
  bitcoind process and delegates unrecognised messages to the RPC
  connection.

  This commit changes start_nodes and stop_nodes to start and stop the
  bitcoind nodes in parallel, making test setup and teardown much faster.

  On my vm, this changeset reduces total test_runner runtime for the base set of tests
  (including building the cache) from 263s to 195s (a 25% speedup). Note that the time
  reported by test_runner does not include time spent building the cache:

  *with TestNode*:
  ```
  → date +"%T" ; ./test_runner.py -q ; date +"%T"
  12:48:04
  ..................................................................................................................................................................................................................................................................................................................................
  TEST                           | STATUS    | DURATION

  abandonconflict.py             | ✓ Passed  | 12 s
  bip68-112-113-p2p.py           | ✓ Passed  | 19 s
  blockchain.py                  | ✓ Passed  | 8 s
  bumpfee.py                     | ✓ Passed  | 13 s
  decodescript.py                | ✓ Passed  | 3 s
  disablewallet.py               | ✓ Passed  | 3 s
  disconnect_ban.py              | ✓ Passed  | 6 s
  fundrawtransaction.py          | ✓ Passed  | 37 s
  getchaintips.py                | ✓ Passed  | 4 s
  httpbasics.py                  | ✓ Passed  | 3 s
  import-rescan.py               | ✓ Passed  | 4 s
  importmulti.py                 | ✓ Passed  | 6 s
  importprunedfunds.py           | ✓ Passed  | 3 s
  invalidblockrequest.py         | ✓ Passed  | 4 s
  invalidtxrequest.py            | ✓ Passed  | 4 s
  keypool.py                     | ✓ Passed  | 7 s
  listsinceblock.py              | ✓ Passed  | 4 s
  listtransactions.py            | ✓ Passed  | 33 s
  mempool_limit.py               | ✓ Passed  | 4 s
  mempool_persist.py             | ✓ Passed  | 15 s
  mempool_reorg.py               | ✓ Passed  | 4 s
  mempool_resurrect_test.py      | ✓ Passed  | 3 s
  mempool_spendcoinbase.py       | ✓ Passed  | 3 s
  merkle_blocks.py               | ✓ Passed  | 3 s
  multi_rpc.py                   | ✓ Passed  | 4 s
  net.py                         | ✓ Passed  | 3 s
  nulldummy.py                   | ✓ Passed  | 3 s
  p2p-compactblocks.py           | ✓ Passed  | 28 s
  p2p-fullblocktest.py           | ✓ Passed  | 126 s
  p2p-leaktests.py               | ✓ Passed  | 8 s
  p2p-mempool.py                 | ✓ Passed  | 3 s
  p2p-segwit.py                  | ✓ Passed  | 59 s
  p2p-versionbits-warning.py     | ✓ Passed  | 8 s
  preciousblock.py               | ✓ Passed  | 3 s
  prioritise_transaction.py      | ✓ Passed  | 5 s
  proxy_test.py                  | ✓ Passed  | 3 s
  rawtransactions.py             | ✓ Passed  | 9 s
  receivedby.py                  | ✓ Passed  | 19 s
  reindex.py                     | ✓ Passed  | 12 s
  rest.py                        | ✓ Passed  | 9 s
  rpcnamedargs.py                | ✓ Passed  | 3 s
  segwit.py                      | ✓ Passed  | 7 s
  sendheaders.py                 | ✓ Passed  | 24 s
  signmessages.py                | ✓ Passed  | 3 s
  signrawtransactions.py         | ✓ Passed  | 3 s
  txn_clone.py                   | ✓ Passed  | 4 s
  txn_doublespend.py --mineblock | ✓ Passed  | 4 s
  uptime.py                      | ✓ Passed  | 3 s
  wallet-accounts.py             | ✓ Passed  | 3 s
  wallet-dump.py                 | ✓ Passed  | 7 s
  wallet-encryption.py           | ✓ Passed  | 8 s
  wallet-hd.py                   | ✓ Passed  | 15 s
  wallet.py                      | ✓ Passed  | 31 s
  walletbackup.py                | ✓ Passed  | 104 s
  zapwallettxes.py               | ✓ Passed  | 9 s
  zmq_test.py                    | ○ Skipped | 0 s

  ALL                            | ✓ Passed  | 735 s (accumulated)
  Runtime: 189 s

  12:51:19
  ```

  *master*:
  ```
  → date +"%T" ; ./test_runner.py -q ; date +"%T"
  12:40:13
  ..........................................................................................................................................................................................................................................................................................................................................................................................................................................
  TEST                           | STATUS    | DURATION

  abandonconflict.py             | ✓ Passed  | 15 s
  bip68-112-113-p2p.py           | ✓ Passed  | 19 s
  blockchain.py                  | ✓ Passed  | 8 s
  bumpfee.py                     | ✓ Passed  | 20 s
  decodescript.py                | ✓ Passed  | 3 s
  disablewallet.py               | ✓ Passed  | 3 s
  disconnect_ban.py              | ✓ Passed  | 8 s
  fundrawtransaction.py          | ✓ Passed  | 36 s
  getchaintips.py                | ✓ Passed  | 11 s
  httpbasics.py                  | ✓ Passed  | 7 s
  import-rescan.py               | ✓ Passed  | 16 s
  importmulti.py                 | ✓ Passed  | 10 s
  importprunedfunds.py           | ✓ Passed  | 5 s
  invalidblockrequest.py         | ✓ Passed  | 4 s
  invalidtxrequest.py            | ✓ Passed  | 3 s
  keypool.py                     | ✓ Passed  | 7 s
  listsinceblock.py              | ✓ Passed  | 11 s
  listtransactions.py            | ✓ Passed  | 37 s
  mempool_limit.py               | ✓ Passed  | 4 s
  mempool_persist.py             | ✓ Passed  | 23 s
  mempool_reorg.py               | ✓ Passed  | 7 s
  mempool_resurrect_test.py      | ✓ Passed  | 3 s
  mempool_spendcoinbase.py       | ✓ Passed  | 3 s
  merkle_blocks.py               | ✓ Passed  | 10 s
  multi_rpc.py                   | ✓ Passed  | 6 s
  net.py                         | ✓ Passed  | 6 s
  nulldummy.py                   | ✓ Passed  | 3 s
  p2p-compactblocks.py           | ✓ Passed  | 30 s
  p2p-fullblocktest.py           | ✓ Passed  | 126 s
  p2p-leaktests.py               | ✓ Passed  | 8 s
  p2p-mempool.py                 | ✓ Passed  | 3 s
  p2p-segwit.py                  | ✓ Passed  | 62 s
  p2p-versionbits-warning.py     | ✓ Passed  | 8 s
  preciousblock.py               | ✓ Passed  | 8 s
  prioritise_transaction.py      | ✓ Passed  | 7 s
  proxy_test.py                  | ✓ Passed  | 10 s
  rawtransactions.py             | ✓ Passed  | 15 s
  receivedby.py                  | ✓ Passed  | 28 s
  reindex.py                     | ✓ Passed  | 12 s
  rest.py                        | ✓ Passed  | 12 s
  rpcnamedargs.py                | ✓ Passed  | 3 s
  segwit.py                      | ✓ Passed  | 12 s
  sendheaders.py                 | ✓ Passed  | 26 s
  signmessages.py                | ✓ Passed  | 3 s
  signrawtransactions.py         | ✓ Passed  | 3 s
  txn_clone.py                   | ✓ Passed  | 10 s
  txn_doublespend.py --mineblock | ✓ Passed  | 10 s
  uptime.py                      | ✓ Passed  | 3 s
  wallet-accounts.py             | ✓ Passed  | 3 s
  wallet-dump.py                 | ✓ Passed  | 6 s
  wallet-encryption.py           | ✓ Passed  | 8 s
  wallet-hd.py                   | ✓ Passed  | 18 s
  wallet.py                      | ✓ Passed  | 69 s
  walletbackup.py                | ✓ Passed  | 130 s
  zapwallettxes.py               | ✓ Passed  | 15 s
  zmq_test.py                    | ○ Skipped | 0 s

  ALL                            | ✓ Passed  | 936 s (accumulated)
  Runtime: 242 s

  12:44:36
  ```

Tree-SHA512: 6dfc4c11fd0caf7de6954c93679cf22c3df0acc6f432e616d1151062a61f456faa8ae2fe670b427868af55bb564802df84c8fd76e90b4b338750dbc23f46ad88
@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Sep 2, 2017

@jnewbery Should this be kept open?

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Sep 4, 2017

Perhaps if/when #11182 is merged we can close this? (or we can close now if you feel strongly)

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Sep 5, 2017

Going to close this. People interested should focus on reviewing the last bit, i.e. #11182.

@MarcoFalke MarcoFalke closed this Sep 5, 2017

MarcoFalke added a commit that referenced this pull request Nov 8, 2017

Merge #11182: [tests] Add P2P interface to TestNode
32ae82f [tests] use TestNode p2p connection in tests (John Newbery)
5e5725c [tests] Add p2p connection to TestNode (John Newbery)
b86c1cd [tests] fix TestNode.__getattr__() method (John Newbery)

Pull request description:

  Final two steps of #10082 : Adding the "mininode" P2P interface to `TestNode`

  This PR adds the mininode P2P interface to `TestNode`. It simplifies the process for opening a P2P connection to the node-under-test from this:

  ```python
  node0 = NodeConnCB()
  connections = []
  connections.append(NodeConn('127.0.0.1', p2p_port(0), self.nodes[0], node0))
  node0.add_connection(connections[0])
  ```

  to this:

  ```python
  self.nodes[0].add_p2p_connection(p2p_conn_type=NodeConnCB)
  ```

  The first commit adds the infrastructure to `test_node.py`. The second updates the individual test cases to use it. Can be separated if this is too much review for one PR.

Tree-SHA512: 44f1a6320f44eefc70489ae8350c6a16ad1a6035e4b9b7bafbdf19f5905ed0e2db85beaaf4758eec3059dd89a375a47a45352a029f39f57a86ab38a9ae66650e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.