Added TestShell class for interactive Python environments. #17288

jachiang commented at 7:17 PM on October 28, 2019: contributor

This PR refactors BitcoinTestFramework to encapsulate setup and shutdown logic into dedicated methods, and adds a ~~TestWrapper~~ TestShell child class. This wrapper allows the underlying BitcoinTestFramework to run between user inputs in a REPL environment, such as a Jupyter notebook or any interactive Python3 interpreter.

The ~~TestWrapper~~ TestShell is motivated by the opportunity to expose the test-framework as a prototyping and educational toolkit. Examples of code prototypes enabled by ~~TestWrapper~~ TestShell can be found in the Optech Taproot/Schnorr workshop repository.

Usage example:

>>> import sys
>>> sys.path.insert(0, "/path/to/bitcoin/test/functional")

>>> from test_framework.test_wrapper import TestShell
>>> test = TestShell()
>>> test.setup(num_nodes=2)
20XX-XX-XXTXX:XX:XX.XXXXXXX TestFramework (INFO): Initializing test directory /path/to/bitcoin_func_test_XXXXXXX

>>> test.nodes[0].generate(101)
>>> test.nodes[0].getblockchaininfo()["blocks"]
101

>>> test.shutdown()
20XX-XX-XXTXX:XX:XX.XXXXXXX TestFramework (INFO): Stopping nodes
20XX-XX-XXTXX:XX:XX.XXXXXXX TestFramework (INFO): Cleaning up /path/to/bitcoin_func_test_XXXXXXX on exit
20XX-XX-XXTXX:XX:XX.XXXXXXX TestFramework (INFO): Tests successful

Overview of changes to BitcoinTestFramework:

Code moved to setup()/shutdown() methods.
Argument parsing logic encapsulated by parse_args method.
Success state moved to BitcoinTestFramework.success.

During Shutdown

BitcoinTestFramework logging handlers are flushed and removed.
BitcoinTestFrameowork.nodes list is cleared.
NetworkThread.network_event_loop is reset. (NetworkThread class).

Behavioural changes:

Test parameters can now also be set when overriding BitcoinTestFramework.setup() in addition to overriding set_test_params method.
Potential exceptions raised in BitcoinTestFramework.setup() will be handled in main().

Added files:

~~test_wrapper.py~~ test_shell.py
~~test-wrapper.md~~ test-shell.md

fanquake added the label Tests on Oct 28, 2019

fanquake requested review from MarcoFalke on Oct 28, 2019

jachiang commented at 7:19 PM on October 28, 2019: contributor

@jnewbery has provided many inputs for this PR. Previous discussion and review can be found here.

in doc/test-wrapper.md:1 in 9535d81cae outdated

   0 | @@ -0,0 +1,123 @@
   1 | +Test Wrapper for Interactive Environments

MarcoFalke commented at 7:35 PM on October 28, 2019:

nit: Wouldn't it make more sense to put this file in the test/ directory? I feel like we already have some documentation there and it could be referenced from the existing docs.

jonatack commented at 11:55 AM on October 29, 2019:

...or possibly in test/functional/

jnewbery commented at 7:50 PM on October 28, 2019: member

Concept ACK. We used this in the taproot workshops and it's a really nice way of interacting with bitcoind instances, prototyping tests, learning the RPC interface, etc

jamesob commented at 8:03 PM on October 28, 2019: member

Concept ACK, seems like a great feature.

practicalswift commented at 10:05 PM on October 28, 2019: contributor

Concept ACK: very nice idea and what an excellent first-time contribution!

Hope to see more contributions from you in the future!

in doc/test-wrapper.md:6 in 9535d81cae outdated

   0 | @@ -0,0 +1,123 @@
   1 | +Test Wrapper for Interactive Environments
   2 | +=========================================
   3 | +
   4 | +This document describes the usage of the `TestWrapper` submodule in the functional test suite.
   5 | +
   6 | +The TestWrapper submodule extends the `BitcoinTestFramework` functionality to external interactive environments for prototyping and educational purposes. Just like `BitcoinTestFramework`, the TestWrapper allows the user to:

jonatack commented at 11:56 AM on October 29, 2019:

In general, it would be nice to wrap lines in markdown files around 80-120 characters for easier reviewing here and in local editors. Most editors can do this automatically for you while editing the file and it would be kind for reviewers.

in doc/test-wrapper.md:10 in 9535d81cae outdated

   5 | +
   6 | +The TestWrapper submodule extends the `BitcoinTestFramework` functionality to external interactive environments for prototyping and educational purposes. Just like `BitcoinTestFramework`, the TestWrapper allows the user to:
   7 | +
   8 | +* Manage regtest bitcoind subprocesses.
   9 | +* Access RPC interfaces of the underlying bitcoind instances.
  10 | +* Log events to functional the test logging utility.

jonatack commented at 11:57 AM on October 29, 2019:

s/functional the/the functional/

in doc/test-wrapper.md:12 in 9535d81cae outdated

   7 | +
   8 | +* Manage regtest bitcoind subprocesses.
   9 | +* Access RPC interfaces of the underlying bitcoind instances.
  10 | +* Log events to functional the test logging utility.
  11 | +
  12 | +The `TestWrapper` can be useful in interactive environments where it is necessary to extend the object lifetime of the underlying `BitcoinTestFramework` between user inputs. Such environments include the Python3 command-line interpreter or [Jupyter](https://jupyter.org/) notebooks running a Python3 kernel,

jonatack commented at 11:58 AM on October 29, 2019:

nit: s/command-line/command line/

jonatack commented at 11:58 AM on October 29, 2019:

s/kernel,/kernel./ ?

in doc/test-wrapper.md:26 in 9535d81cae outdated

  21 | +We can import the TestWrapper by adding the path of the Bitcoin Core `test_framework` module to the beginning of the PATH variable, and then importing the `TestWrapper` class from the `test_wrapper` sub-package.
  22 | +
  23 | +```
  24 | +>>> import sys
  25 | +>>> sys.path.insert(0, "/path/to/bitcoin/test/functional")
  26 | +>>> from test_framework.test_wrapper import TestWrapper

jonatack commented at 12:00 PM on October 29, 2019:

The first 2 commands didn't seem needed for running your example in the PR description?

jachiang commented at 4:09 PM on October 30, 2019:

Thank you, I have added it to the PR description.

in doc/test-wrapper.md:38 in 9535d81cae outdated

  33 | +
  34 | +The TestWrapper inherits all BitcoinTestFramework members and methods, such as:
  35 | +* `TestWrapper.nodes[index].rpc_method()`
  36 | +* `TestWrapper.log.info("Custom log message")`
  37 | +
  38 | +The following sections demonstrate how to initialize, run and shutdown a TestWrapper object.

jonatack commented at 12:00 PM on October 29, 2019:

nit: s/run/run,/ (add Oxford comma) and s/shutdown/shut down/ (shutdown is a noun but not a verb)

in doc/test-wrapper.md:50 in 9535d81cae outdated

  45 | +20XX-XX-XXTXX:XX:XX.XXXXXXX TestFramework (INFO): Initializing test directory /path/to/bitcoin_func_test_XXXXXXX
  46 | +```
  47 | +The TestWrapper forwards all functional test parameters of the parent Bitcoin TestFramework object. The full set of argument keywords which can be used to initialize the TestWrapper can be found [here](../test/functional/test_framework/test_wrapper.py).
  48 | +
  49 | +**Note: Running multiple instances of TestWrapper is not allowed.**
  50 | +This also ensures that logging remains consolidated in the same temporary folder. If you need more bitcoind nodes than set by default (1), simply increase the `num_nodes` parameter during setup.

jonatack commented at 12:02 PM on October 29, 2019:

Perhaps s/This/Running a single process/

in doc/test-wrapper.md:60 in 9535d81cae outdated

  55 | +TestWrapper is already running!
  56 | +```
  57 | +
  58 | +## 4. Interacting with the TestWrapper
  59 | +
  60 | +Unlike the BitcoinTestFramework class, the TestWrapper keeps the underlying Bitcoind subprocesses (nodes) and logging utilities running, until the user explicitly shuts down the TestWrapper object.

jonatack commented at 12:03 PM on October 29, 2019:

s/running,/running/ (remove the comma)

in doc/test-wrapper.md:100 in 9535d81cae outdated

  95 | +```
  96 | +>>> test.nodes[0].log.info("Successfully mined regtest chain!")
  97 | +20XX-XX-XXTXX:XX:XX.XXXXXXX TestFramework.node0 (INFO): Successfully mined regtest chain!
  98 | +```
  99 | +
 100 | +**Note: Please also consider the functional test [readme](../test/functional/README.md), which provides an overview of the test-framework**. Modules such as [key.py](../test/functional/test_framework/key.py), [script.py](../test/functional/test_framework/script.py) and [messages.py](../test/functional/test_framework/messages.py) are especially useful in constructing objects which can be passed to the bitcoind nodes managed by a running TestWrapper object.

jonatack commented at 12:05 PM on October 29, 2019:

nit: s/especially/particularly/

in test/functional/test_framework/test_framework.py:167 in 9535d81cae outdated

 162 | @@ -135,6 +163,9 @@ def main(self):
 163 |          self.add_options(parser)
 164 |          self.options = parser.parse_args()
 165 |  
 166 | +    def setup(self):
 167 | +        """Call this method to startup the test-framework object with options set."""

jonatack commented at 12:07 PM on October 29, 2019:

nit: s/startup/start up/ and s/test-framework/test framework/

in test/functional/test_framework/test_framework.py:226 in 9535d81cae outdated

 246 | -            self.log.warning("Exiting after keyboard interrupt")
 247 | +        self.success = TestStatus.PASSED
 248 |  
 249 | -        if success == TestStatus.FAILED and self.options.pdbonfailure:
 250 | +    def shutdown(self):
 251 | +        """Call this method to shutdown the test-framework object."""

jonatack commented at 12:08 PM on October 29, 2019:

nit: s/shutdown/shut down/ and s/test-framework/test framework/

in test/functional/test_framework/test_wrapper.py:23 in 9535d81cae outdated

  18 | +from test_framework.test_framework import BitcoinTestFramework
  19 | +
  20 | +class TestWrapper:
  21 | +
  22 | +    class __TestWrapper(BitcoinTestFramework):
  23 | +

jonatack commented at 12:10 PM on October 29, 2019:

nit: possibly remove the extra lines at L21, 23 and 31

in test/functional/test_framework/test_wrapper.py:36 in 9535d81cae outdated

  31 | +
  32 | +            if self.running:
  33 | +                print("TestWrapper is already running!")
  34 | +                return
  35 | +
  36 | +            self.setup_clean_chain = kwargs.get('setup_clean_chain',True)

jonatack commented at 12:12 PM on October 29, 2019:

Do these args need to be maintained in parallel with those in test_framework.py, etc? If yes, perhaps mention/warn about it in a code comment.

in doc/test-wrapper.md:123 in 9535d81cae outdated

 118 | +20XX-XX-XXTXX:XX:XX.XXXXXXX TestFramework (INFO): Tests successful
 119 | +```
 120 | +
 121 | +The following utility consolidates logs from the bitcoind nodes and the underlying BitcoinTestFramework:
 122 | +
 123 | +* `/path/to/bitcoin/test/functional/combine_logs.py '/path/to/bitcoin_func_test_XXXXXXX'`

jonatack commented at 12:18 PM on October 29, 2019:

nit: missing newline at EOF

jonatack commented at 12:19 PM on October 29, 2019: member

ACK 9535d81caeac13b9e8180e5c3d4b60190f030629 modulo nits.

Nice feature. I'm not sure why some of the commits are separate. A few comments below for your perusal. Feel free to ignore the nits.

Light code review, built, ran tests, ran individual functional tests from root and from test/functional and also using a few different options (e.g. -l, --filter).

I did not test edge cases involving the exceptions.

Ran the following python code successfully from /test/functional:

~/projects/bitcoin/bitcoin/test/functional (pr/17288)$ python3
Python 3.7.3 (default, Apr  3 2019, 05:39:12)
[GCC 8.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from test_framework.test_wrapper import TestWrapper
>>> test = TestWrapper()
>>> test.setup(num_nodes=2)
2019-10-29T11:39:17.513000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_6czimoav
>>> test.nodes[0].generate(101)
['6296f563a83ae9fbe33f450d33ffb3da5ab737e52beef8083b7c8f16d13c0158', '240bf8d445439847a63d1778b418204bd031cbf8c893ffa548cab167ccd28545', '168fd28ce6a4a86fe886245e56763fa214b21918ce08f3ad060659b1420e2805', '43d92b96defb72244d3c815261992d0126b57d13e34ab0c6029aec80f084495f', '75e6c11a548aef554ccdbfdf6e2e37738d9f53f2fe2698f53ea16d4af96539c9', '200be654e7303b88fa71053dde246c08bc811ffbb2f4b08554c98442d40f3322', '3b3f5e97ec374da169acb3993bad8be5b45f3287fd7e3700899cd5093671bf18', '08650842e4808c9641166b9557bd64723de8c6b7bc55e79fd900253f82af2250', '5dbd416701212809a40048752ec9ffe447a60c2b43a1ccbddcfd635a0f5b13d0', '0f52c30c6f64eb1aa9142fb035b8217d0d6b93c4814d7d9255082089b53f3231', '0bccd3cf893ecea4870fc127ff954f3738d1cc54d9f185b0766d55d90b106fed', '3bffa7736376f5c625b743e9f35fec5544b1bcdf0555d18162cc505eade9e422', '70e7c1aa2f3ebb76cbdc3ec553e3c5d3cb963cebc30696abbc755cc0dd93a049', '6cd0218ecb54e35a8cf9876624f8a8f667abd974517bfafa82b1ef52544f703b', '216c3d991e6714ba2fb1ce59ee1d2412387c45c82a0984d7e2e52e5a3aede42a', '499e7346477155a70f42a4cf42671be55484b46b48cafae4e2fadac55459239a', '3ba894f1d3e4c500783ce9d03b05d14cb70fc5583d8dc67d7cca7c6485f9431d', '2f6bb8f17201153363fdb4c055033b6bbbc3f7f29fe2f9bd97edfc01bbaa35ad', '4fd95b5e51755f811e736e9c14507bd1b7759669bdadd2423dbb8d473b3c2bc1', '136c04a6d3760623b2d7558d4b08a896364d5ee478652b89f0e33e5fe15fda8c', '451d8bfc910cc39a58c761d8e943139df2e619ecc6bf61a84e402155eb79a9ba', '05fd7533b818312772d04c0d5e074ed28dc78f94027fe4f434990c2a03d58995', '3ec24df6f72dab954dc2efa71072f2f55a717abb672c911faff799ec35bd2ef1', '09ba7d1167b5c0e97d6afc41f666c2769ccd7b7b30ef68b7ae1929cea53c0c21', '30d113bf788a1352ccd6b4b91be50932280a0bdaef4088922f86502dc622781a', '4bb118b999d38c0e4e242b71a3f7f08a28012f184b13668ca8d03fd6d545c033', '5659f3bd02616bc25d8f71b9683e55d709a34cc1b5dca1cdb7a2d939d6257aa3', '4f7bb9fd15f2685fcb0186eaf6194b4fc503f7cfaadcd3b9b0c2154abc9a3107', '5434b651df36c0b751031f449f67fe1bc05f517884a9d83b97482ae352227234', '5660c66f1cdd65bfb3cdecb0861863112cec1f50d473dfdd16d7bda8cbc3a8e5', '5366d1708b3ca35b85b82e740d54c551602f5502e7d8d1d5d57c92c86308f0e2', '49a7c184dd5cc5947d77d9bb69e3a7bba3b09c0f618e7e2f82138b26e35f38aa', '0264898e262228b783096ac9247abd49442bea11ccb14df0ac384c6c3da71439', '47478790ae3c4adc4d2ef6f7394922fb964211cc4dd36a1aed0e9c7945048618', '5fc90562a749c1a214aa4fe0b7ba37f53680154c5304c64bb9de989b14233621', '02d8b7b46180df6e029aedb5f512ad238f9ef853960926bf9ba6cdb0d6d4abed', '4f58507e6e241eda3c77da3af1498f210c980922eb48bfe728cc6ccaacbd5e4a', '1883aad8fe99dd242557ac80696cfef3e0febf38683c0f27d58e3a5498ee3947', '364136e959179e42a9072baa321c2b303d41e8cb7211fa0a4fb9f72d198f8681', '247cd9f37fa038baf32159235d67ee9aba7b7ea7f151bfaf4674ab2020137e43', '21701a772f14135b4e5b322ad9010c6507f8814c47b9c5317284dcd06997410c', '1dbde5acf680c9e77c5ca23aa54f5a8c0530804ea5eab17037c26fa4cfdafeaa', '258672d5ff0c3b087a7a872eba546898cedb781c22b8377f11e86eb5dee302c3', '4a6affad25e64ef7ae2a0c6111cd2cf6498760072bbfec8e80f594fd7077dc8c', '3ef480f55878e349230512fa9c9ccde575ef6ef838e2c426dec845735ab75c37', '6e3c76a9c7cfcb7360185833be6bb4cf7a9f98beb3be3c4fd64431bdc0038f4d', '59a4c92d1cc830d7ccb1528e2e4847a3ae093836e84d53adcc66797873f7c064', '59ea70a2847d522a6018185479455ea5c39ac40cefd919bfc32659ed1fe1d70c', '6e04fe094699177e3bb16a50fde3288ea5c01471095d745861f845a84c60876b', '5fb9174bfce3b90433493a25e6736d7b533f970c6173fb642dd244b28f2a57cf', '401d614edfc2b74d2206d9104e5f11c019759f7aeccda2a3013b870af2f243c4', '738e698f817130ed9c853478055d9ebc2299ad1f0a2f67cf38e0166c4755d2e7', '0bd8c2121ee28086a1c38b43106d58f5a71f70a4458cb359dd8029e73c42937a', '71146521878e0c4d2fd0062ce5925b9e3631f5a824bde521883580988eb0a282', '12e5f3523adf19f2b62909ae582edcb20b407b1aee416ab658211ed00dbcbb4d', '0e6433333299ca958140f3e50595c77a504d6b3136adcbb812cfc662a689bfeb', '79f959dee278330d30cda0e04c7aeb8242236c8d217f1fa0d2a521afb0789954', '0899237c2c67488bc5d2c858f5b72477abc4aa932b9f1504af200238cd97ad7d', '75647104030f9c738b400901e8aa95243a7529c95ae30e3ba489c3c232fb502f', '10861a045967752d885c0ddb368457e75d438f7412d832348fd03e69ae7b25bc', '6e9fc94e05112ad5208eeca1f0fab19c6b4c4a5ed1b6835ef1496615bdcbda82', '6fe3223defbdbe8312b3ec18139f4b8b6ca0f8583f95f1dfcc2e02f91c8dd308', '3d9ba51969161af66add077fd6f18d9312863eed3f765a6255b15e9cb9bcd471', '7a59caef29ecedb139202ae1ae44f66c0dfb3de66447045c20a5c1aec92cf121', '11d0cc5ac92c25bd7fd9a2a813162b78b116f029f26e72165ed02342d0660c6d', '2682bffc07d85e076637a20c9f0607d984862483bf5b60ab65efacca7328b237', '04afce02b95dd44d7ad27b9e1607fbefd63a24d23fead6e109c75d3f2703b641', '50fcb897a23eee74e607812d6d1c4e6c9b49470c94e7d2e8b6102d77d68280ce', '0fd2a9a24a87428bf73053bfc422fb2b24c01ba77a1dd74f230a028c4cef0e5e', '752b8503cd8707c83772c10132b0d2b4468b7d30102fbfcfa1b4dfe5367a6ee6', '1e24cd97d61dc62bb9c7e98bb9ee5b329d3063cceac65d79d30a1fa64caf317f', '4fdcf4a595114ae5cfcc76927cdcdec5dc1ad9e912212450d2da353f1cf7a4fa', '4f30222ca0ac5d6d599a108803523445098ba4db7a1004e1118b74eca9ea3bf8', '5ef1bdba640205e925884a4bf608ad3f483577f65cde056576b4b1896262595b', '73d410de2a4c49021e900a1ff3274cacda80af5093dff50185e3de5190eb0cd9', '1c6f4fccbee8342f3228d35db71ed1de14b64b7183442c5cdb4876fae983ed83', '3bf5924c37c40c1d0a82e7d595ce3c2d3d915bc98ad734fc786d59dafe74f65b', '1d467362b5bfab4257cc3e4e06308fb6c017b4783e1a601d8d1b32d6b92b667f', '015b7e163da7a0fd6adf8844c5fba3b791ad430e7d33019f02d1f4f2713fc321', '68453df232e2bb1a5f221377d738f6933431d16d42e28882fb98de910056510e', '160cf3ed34e3eeb4cf8a605eb7e720fb19deaffe8cc37c33389e876cb65fbf7b', '04ec683a1fb556c027baff8f7ff3297466b2ea5ddf3dfa3483602307a8f8e07c', '2615f152d40768eab58a84fbbaf8f8873b633b44368a28c7f06cc01f20fe8bfb', '7b1508a1414b67f0c76d7b1758b3a45cac50ec1b778d9ac9f97a411dcab02c41', '5ca8734137cc13fa185e3fc7f789f25f81ada8ebb4cc3d3944d5995e3ee6fc21', '4bb2ec1cd9f866458d767de4be969e659dd4474f102555991e98ac3e990a1ef1', '4215d168b3e139da0d655cc732dee825e7404a29e12d0a1cbeea56bd79ebff8f', '5f17bb8262f8e1e91648ee5d4862781b0ebd0a1a6a945f43738b4c6fe06aab38', '30cbe481acd815c4a18b849bbde8e69246ee40bb30dd88298a195b362ae4418b', '10585b742a0aee7954223c3724127aa74042cee1a2fa029d6f868f1bb5977b88', '35a0ee0ec0368eb8e26f121212aa25f495539c2120c15ca5690bf4a273dbb7b8', '28b10f505ee44d2448e0eba5e379a22b06a93e61b0861ec322819e1da1393ffc', '09d815b4c872c6d4bb5a31c1586c976d239f493d628224f8a01d8c7c20b4a37c', '5a40268f5791b40a7292f805ffb801b0b74511e29757161f75be70c0c2969e58', '243b3d49454bc1dbe9f6770edd9f23eab0886db96e98c3f54d8b52396dc780c7', '48e4904b36d10e66a305a8d4563283aa46ce4988365cf4ba955e186bfdb774af', '2f69866be1bca6cc05d0c2f1b150d780b3166daf82644bd336b22c8e0edf6dd2', '23827cae8f0a27f64ca6417f1ad2b908cf0033a3d9b33357b96cdadad2cc4748', '7b6c1957112dbfaa60d98b9a15b25d6ecc0fc133f9fa9a4943664b16cd46993e', '4bc98ca727d9cc98ce977a096ac4404d3560c2a3c53768124cf6431f3e8bc206', '12e3c9b6155fa20bd100c8e8b0c670f4f8c4cacd41739d4a54e4aadae01547f8']
>>> test.nodes[0].getblockchaininfo()["blocks"]
101
>>> test.nodes[0].getnewaddress()
'bcrt1q86scpkn2zdrdmrgnx787n25n983xprp5x3jxnv'
>>> test.nodes[0].getwalletinfo()
{'walletname': '', 'walletversion': 169900, 'balance': Decimal('50.00000000'), 'unconfirmed_balance': Decimal('0E-8'), 'immature_balance': Decimal('5000.00000000'), 'txcount': 101, 'keypoololdest': 1572349178, 'keypoolsize': 0, 'keypoolsize_hd_internal': 1, 'paytxfee': Decimal('0E-8'), 'hdseedid': 'ffa977f7711472819ad31b72d006872df4ce4052', 'private_keys_enabled': True, 'avoid_reuse': False, 'scanning': False}
>>> test.shutdown()
2019-10-29T11:39:48.079000Z TestFramework (INFO): Stopping nodes
2019-10-29T11:39:48.448000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_6czimoav on exit
2019-10-29T11:39:48.448000Z TestFramework (INFO): Tests successful

fjahr commented at 7:23 PM on October 29, 2019: member

Concept ACK

I can see how this will be useful for a lot of people in the future. Only one nit: I am not sure if TestWrapper is a great name. I would expect a class by that name to wrap a specific test. But it's hard to come up with a better name that is still short and describes what this class is doing. Maybe something like InteractiveDriver. But probably not worth the effort unless others agree it should be changed.

jnewbery commented at 7:26 PM on October 29, 2019: member

I am not sure if TestWrapper is a great name.

I think the name is ok. Other options: TestREPL, TestShell, TestDriver, ...

jnewbery commented at 8:09 PM on October 29, 2019: member

In general, it would be nice to wrap lines in markdown files around 80-120

Please also do the same for commit logs! Aim to limit the summary line to 50 characters if possible and wrap all other lines at 80 characters. There are some good pointers on writing commit logs here: https://chris.beams.io/posts/git-commit/

in test/functional/test_framework/test_framework.py:272 in 9535d81cae outdated

 267 |          else:
 268 |              self.log.error("Test failed. Test logging available at %s/test_framework.log", self.options.tmpdir)
 269 |              self.log.error("Hint: Call {} '{}' to consolidate all logs".format(os.path.normpath(os.path.dirname(os.path.realpath(__file__)) + "/../combine_logs.py"), self.options.tmpdir))
 270 |              exit_code = TEST_EXIT_FAILED
 271 | -        logging.shutdown()
 272 | +        for h in list(self.log.handlers):

jnewbery commented at 8:20 PM on October 29, 2019:

I think this could use a comment saying why you're not just calling logging.shutdown() (maybe reference https://docs.python.org/3/library/logging.html#logging.shutdown).

Without a comment, someone might helpfully change this back to using logging.shutdown() in future.

in test/functional/test_framework/test_wrapper.py:7 in 9535d81cae outdated

   0 | @@ -0,0 +1,83 @@
   1 | +#!/usr/bin/env python3
   2 | +# Copyright (c) 2014-2019 The Bitcoin Core developers
   3 | +# Distributed under the MIT software license, see the accompanying
   4 | +# file COPYING or http://www.opensource.org/licenses/mit-license.php.
   5 | +"""Wrapper Class for BitcoinTestFramework.
   6 | +
   7 | +The TestWrapper class extends the BitcoinTestFramework

jnewbery commented at 8:22 PM on October 29, 2019:

I'd move this to be a class docstring for TestWrapper rather than a file docstring.

in test/functional/test_framework/test_wrapper.py:2 in 9535d81cae outdated

   0 | @@ -0,0 +1,83 @@
   1 | +#!/usr/bin/env python3
   2 | +# Copyright (c) 2014-2019 The Bitcoin Core developers

jnewbery commented at 8:23 PM on October 29, 2019:

jachiang commented at 3:14 PM on October 30, 2019:

Almost feels like it ;)

in test/functional/test_framework/test_wrapper.py:73 in 9535d81cae outdated

  68 | +                super().shutdown()
  69 | +                self.running = False
  70 | +
  71 | +    instance = None
  72 | +
  73 | +    def __new__(cls):

jnewbery commented at 8:37 PM on October 29, 2019:

Small comment here to explain that we wrap the class to enforce a singleton pattern would be nice.

jnewbery commented at 8:39 PM on October 29, 2019: member

Great work with the documentation. Thanks for adding that!

I think this could be really useful when combined with a REPL with session logging (eg https://ipython.readthedocs.io/en/stable/interactive/reference.html#session-logging-and-restoring) to quickly prototype a test, and then turn it into a permanent test case.

in test/functional/test_framework/test_wrapper.py:33 in 5828e9a0fd outdated

  28 | +            pass
  29 | +
  30 | +        def setup(self, **kwargs):
  31 | +
  32 | +            if self.running:
  33 | +                print("TestWrapper is already running!")

jamesob commented at 9:04 PM on October 29, 2019:

Make any sense to log instead of print? (same question for below)

jachiang commented at 3:14 PM on October 30, 2019:

Thanks @jamesob for the review. For my understanding, do you mean logging it to the (already) running TestShell/Driver instance?

jnewbery commented at 6:36 PM on October 31, 2019:

I think this is better as a print() which gets displayed to the user immediately. The log context is for what's happening inside the test, not for a user accidentally re-calling the setup message.

jachiang commented at 7:50 AM on November 4, 2019:

Ok, as of now I have left all TestWrapper warnings as print(). These include the following. As far as I can tell, they are specific to the lifetime of the TestShell object and not the underlying test.

"TestShell is already running!" (Multiple initializations)
"TestShell is not running!" (Shutdown call without running test)
"Shutdown TestWrapper before resetting!" (Reset call while running test)

jamesob commented at 9:10 PM on October 29, 2019: member

Good change, agree with others' nits though. TestShell or TestDriver might be clearer but no big deal.

laanwj commented at 11:54 AM on October 30, 2019: member

Concept ACK

practicalswift commented at 12:06 PM on October 30, 2019: contributor

Regarding the naming: TestShell or TestDriver as suggested by @jnewbery and echoed by @jamesob are both clearer names than TestWrapper IMO :)

jachiang commented at 3:21 PM on October 30, 2019: contributor

Regarding the naming: TestShell or TestDriver as suggested by @jnewbery and echoed by @jamesob are both clearer names than TestWrapper IMO :)

Many thanks for all the naming ideas, so it seems like TestShell / TestDriver are the leading candidates? (I sorta like TestShell because it implies an interactive interface?)

jonatack commented at 4:35 PM on October 30, 2019: member

Since we're discussing names, can offer up TestConsole as well (and give some love to the TestREPL suggestion.)

MarcoFalke commented at 5:24 PM on October 30, 2019: member

Ok, let's go ahead and pick a name. "Wrapper" is a bit too general and I am fine with any of the previous other suggestions.

jachiang force-pushed on Nov 3, 2019

Remove network_event_loop instance in close()

The asyncio.new_event_loop() instance is now removed from the NetworkThread
class during shutdown. This enables a NetworkThread instance to be restarted
after being closed. The current NetworkThread class guards against an existing
new_event_loop during initialization.

ede8b7608e

Refactor TestFramework main() into setup/shutdown

Setup and shutdown code now moved into dedicated methods. Test "success" is
added as a BitcoinTestFramework member, which can be accessed outside of main.
Argument parsing also moved into separate method and called from main.

6b71241291

Add closing and flushing of logging handlers

In order for BitcoinTestFramework to correctly restart after shutdown, the
previous logging handlers need to be removed, or else logging will continue in
the previous temp directory. "Flush" ensures buffers are emptied, and "close"
ensures file handler close logging file.

6f40820757

Clear TestNode objects after shutdown

TestNode objects need to be removed during shutdown, as setup_nodes does not
remove previous TestNode objects from previous test runs during setup.

614c645643

Move assert num_nodes is set into main()

This allows a BitcoinTestFramework child class to set test parameters in an
overridden setup() rather than in an overridden set_test_params().

2ab01462f4

Move argparse() to init()

This ensures TestFramework default parameters are set before setup is called. A
 child class will therefore have access to defaults when overriding setup.

5155602a63

jachiang force-pushed on Nov 3, 2019

jachiang commented at 8:38 PM on November 3, 2019: contributor

Thank you everybody for your reviews and comments!

Nits and fixes: Have been addressed.
Naming: May I suggest to move ahead with TestShell? TestREPL also seemed popular but awkward in camel case? (As per @fjahr's comment to rename, and all subsequent naming suggestions.)
Documentation: test/functional/README.md now references TestShell as a prototyping tool for tests, and points out the added utility of session logging if available (As per @jnewbery's comment).

May I suggest some additional changes inspired by @jonatack's comment to consolidate settings in a single location.

TestShell now initializes with default test parameters maintained in BitcoinTestFramework. Notable exception is num_nodes, which must be set by a child class and defaults to 1.
TestShell.reset() has been added, which allows the user to reset test parameters back to default values between runs.
test-shell.md now includes an overview of test parameter keys which can be passed to TestShell.setup(key=value). The list excludes portseed and pdbonfailure, which don't seem to apply to this use case.

MarcoFalke renamed this:
~~Added TestWrapper class for interactive Python environments.~~
Added TestShell class for interactive Python environments.
on Nov 3, 2019

Add TestShell class

A BitcoinTestFramework child class which can be imported by an external user or
project. TestShell.setup() initiates an underlying BitcoinTestFramework object
with bitcoind subprocesses, rpc interfaces and test logging.
TestShell.shutdown() safely tears down the BitcoinTestFramework object.

f5112369cf

in test/functional/test_framework/test_shell.py:56 in e521ff6c9c outdated

  51 | +                super().shutdown()
  52 | +                self.running = False
  53 | +
  54 | +        def reset(self):
  55 | +            if self.running:
  56 | +                print("Shutdown TestWrapper before resetting!")

MarcoFalke commented at 10:59 PM on November 3, 2019:

                print("Shutdown TestShell before resetting!")

jachiang commented at 8:52 PM on November 4, 2019:

Thank you. I'll follow-up.

jachiang force-pushed on Nov 4, 2019

in test/functional/README.md:105 in d42616a1bb outdated

  98 | @@ -99,6 +99,16 @@ P2PInterface object and override the callback methods.
  99 |  Examples tests are [p2p_unrequested_blocks.py](p2p_unrequested_blocks.py),
 100 |  [p2p_compactblocks.py](p2p_compactblocks.py).
 101 |  
 102 | +#### Prototyping tests
 103 | +
 104 | +The [`TestShell`](test-shell.md) class exposes the BitcoinTestFramework
 105 | +functionality to interactive Python3 environments, and can be used to prototype

jonatack commented at 12:13 PM on November 4, 2019:

nit: remove comma after environments, as the clause after is not independent.

in test/functional/test-shell.md:4 in d42616a1bb outdated

   0 | @@ -0,0 +1,187 @@
   1 | +Test Shell for Interactive Environments
   2 | +=========================================
   3 | +
   4 | +This document describes the usage of the `TestShell` submodule in the functional

jonatack commented at 12:14 PM on November 4, 2019:

nit: "describes how to use" might be friendlier.

in test/functional/test-shell.md:15 in d42616a1bb outdated

  10 | +
  11 | +* Manage regtest bitcoind subprocesses.
  12 | +* Access RPC interfaces of the underlying bitcoind instances.
  13 | +* Log events to the functional test logging utility.
  14 | +
  15 | +The `TestShell` can be useful in interactive environments where it is necessary

jonatack commented at 12:15 PM on November 4, 2019:

nit: you write TestShell without backticks above and below; ideally settle on one style. Idem for BitcoinTestFramework.

jachiang commented at 3:03 PM on November 4, 2019:

Fixed.

in test/functional/test-shell.md:58 in d42616a1bb outdated

  53 | +>>> test = TestShell()
  54 | +>>> test.setup(num_nodes=2)
  55 | +20XX-XX-XXTXX:XX:XX.XXXXXXX TestFramework (INFO): Initializing test directory /path/to/bitcoin_func_test_XXXXXXX
  56 | +```
  57 | +The TestShell forwards all functional test parameters of the parent Bitcoin
  58 | +TestFramework object. The full set of argument keywords which can be used to

jonatack commented at 12:18 PM on November 4, 2019:

nit: wrote BitcoinTestFramework elsewhere without spaces

jachiang commented at 3:03 PM on November 4, 2019:

Fixed.

in test/functional/test-shell.md:86 in d42616a1bb outdated

  81 | +interactively.
  82 | +
  83 | +**Example: Mining a regtest chain**
  84 | +
  85 | +By default, the TestShell nodes are initialized with a clean chain. This means
  86 | +that each node of the TestShell is initialized with a block height of 0.

jonatack commented at 12:22 PM on November 4, 2019:

I'm seeing 200 blocks by default, is it me?

((HEAD detached at origin/pr/17288))$ python3
Python 3.7.3 (default, Apr  3 2019, 05:39:12) 
[GCC 8.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> from test_framework.test_shell import TestShell
>>> test = TestShell()
>>> test.setup(num_nodes=2)
2019-11-04T12:07:13.890000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_d123oydj
>>> test2 = TestShell()
>>> test2.setup()
TestShell is already running!
>>> test.nodes[0].getblockchaininfo()["blocks"]
200

Second try without instantiating a second shell.

((HEAD detached at origin/pr/17288))$ python3
Python 3.7.3 (default, Apr  3 2019, 05:39:12) 
[GCC 8.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> from test_framework.test_shell import TestShell
>>> test = TestShell()
>>> test.setup(num_nodes=2)
2019-11-04T12:10:24.474000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_su_r79l1
>>> test.nodes[0].getblockchaininfo()["blocks"]
200

jachiang commented at 1:15 PM on November 4, 2019:

Oops, I forgot to update that. The setup_clean_chain default is now False, since we are inheriting it from the BitcoinTestFramework parent class. Will update in docs, Thanks!!

jachiang commented at 3:04 PM on November 4, 2019:

Fixed in documentation:test.setup(num_nodes=2, setup_clean_chain=True).

in test/functional/test-shell.md:122 in d42616a1bb outdated

 116 | +
 117 | +We can also log custom events to the logger.
 118 | +
 119 | +```
 120 | +>>> test.nodes[0].log.info("Successfully mined regtest chain!")
 121 | +20XX-XX-XXTXX:XX:XX.XXXXXXX TestFramework.node0 (INFO): Successfully mined regtest chain!

jonatack commented at 12:22 PM on November 4, 2019:

Logging tested/verified.

in test/functional/test-shell.md:71 in d42616a1bb outdated

  65 | +simply increase the `num_nodes` parameter during setup.
  66 | +
  67 | +```
  68 | +>>> test2 = TestShell()
  69 | +>>> test2.setup()
  70 | +TestShell is already running!

jonatack commented at 12:22 PM on November 4, 2019:

Tested and verified.

in test/functional/test-shell.md:170 in d42616a1bb outdated

 164 | +`TestShell.setup(key=value)`.
 165 | +
 166 | +**Note:** `TestShell.reset()` will reset test parameters to default values and
 167 | +can be called after the TestShell is shut down.
 168 | +
 169 | +| Test parameter key | Default Value | Description |

jonatack commented at 12:29 PM on November 4, 2019:

Nice! I wonder it this table should be in test/README.md which touches on some of these parameters. Best to not maintain docs in two places if feasible.

jachiang commented at 2:48 PM on November 4, 2019:

Hm. I was thinking about that, but not sure of the following two issues if we are to consolidate test parameter documentation.

A) Minor inconsistency in parameter naming:

BitcoinTestFramework command-line args vs BitcoinTestFramework member:
- --tracerpc vs trace_rpc
- --portseed vs port_seed

TestShell just looks up the setup keywords as attributes, so we pass TestShell.setup(trace_rpc=True) for example. I would like to avoid a translation for these two keywords, do you agree?

B) Different parameter passing locations:

For BitcoinTestFramework, some of these params are overridden in set_test_params() and some passed in as command-line arguments, which doesn't make sense for TestShell, since it's interactive. @jonatack Let me know what you think.

jnewbery commented at 6:16 PM on November 4, 2019:

The risk with documenting defaults outside the code is that they might change and the documentation goes out of date.

I tried updating the TestShell.__doc__ string to list all the defaults programatically but that seems like overkill.

jonatack commented at 7:02 PM on November 4, 2019:

@jachiang in reply to your question, given the good state of the PR now and the acks, I would leave it for a possible follow-up.

jonatack commented at 12:32 PM on November 4, 2019: member

Nice improvements @jachiang. Could you update the PR description (e.g. TestShell)? A few comments below but I like where this is going.

Add documentation for test_shell submodule 19139ee034

jachiang force-pushed on Nov 4, 2019

in test/functional/test_framework/test_shell.py:33 in 19139ee034

  28 | +                return
  29 | +
  30 | +            # Num_nodes parameter must be set
  31 | +            # by BitcoinTestFramework child class.
  32 | +            self.num_nodes = kwargs.get('num_nodes', 1)
  33 | +            kwargs.pop('num_nodes', None)

jnewbery commented at 5:45 PM on November 4, 2019:

instead of popping the entry from kwargs here, why not just set:

self.num_nodes = 1

and let the mainline logic below overwrite that if a num_nodes argument has been passed in.

jachiang commented at 7:32 PM on November 4, 2019:

Much better. Fixed. Thank you!

jnewbery commented at 6:17 PM on November 4, 2019: member

ACK 19139ee034d20ebab1b91d3ac13a8eee70b59374

I've added a nit inline, but I think this is ready for merge now.

jamesob commented at 6:45 PM on November 4, 2019: member

ACK https://github.com/bitcoin/bitcoin/pull/17288/commits/19139ee034d20ebab1b91d3ac13a8eee70b59374

jonatack commented at 7:12 PM on November 4, 2019: member

ACK 19139ee034d20ebab1b91d3ac13a8eee70b59374

Verified that setting up with the updated instruction of test.setup(num_nodes=2, setup_clean_chain=True) initializes with a block height of 0.

jachiang force-pushed on Nov 4, 2019

jnewbery commented at 7:37 PM on November 4, 2019: member

Rather than invalidate the three ACKs for a minor nit, can you force push back to 19139ee034d20ebab1b91d3ac13a8eee70b59374 please? I think this PR was ready to merge before your last force push.

jachiang force-pushed on Nov 4, 2019

jachiang commented at 7:52 PM on November 4, 2019: contributor

Rather than invalidate the three ACKs for a minor nit, can you force push back to 19139ee please? I think this PR was ready to merge before your last force push.

Apologies @jnewbery. Back to 19139ee now.

MarcoFalke referenced this in commit bc38bb9a60 on Nov 4, 2019

MarcoFalke merged this on Nov 4, 2019

MarcoFalke closed this on Nov 4, 2019

MarcoFalke commented at 7:57 PM on November 4, 2019: member

Could do the fixup as a follow-up?

instagibbs commented at 8:02 PM on November 4, 2019: member

very nice! post-hom concept ACK

in test/functional/test-shell.md:34 in 19139ee034

  29 | +importing the `TestShell` class from the `test_shell` sub-package.
  30 | +
  31 | +```
  32 | +>>> import sys
  33 | +>>> sys.path.insert(0, "/path/to/bitcoin/test/functional")
  34 | +>>> from test_framework.test_shell import `TestShell`

MarcoFalke commented at 8:10 PM on November 4, 2019:

                                          ^
SyntaxError: invalid syntax

jachiang commented at 8:49 PM on November 4, 2019:

Sorry. I'll follow-up with a fix.

in test/functional/test-shell.md:188 in 19139ee034

 183 | +| `setup_clean_chain` | `False` | Initializes an empty blockchain by default. A 199-block-long chain is initialized if set to `True`. |
 184 | +| `randomseed` | Random Integer | `TestShell.options.randomseed` is a member of `TestShell` which can be accessed during a test to seed a random generator. User can override default with a constant value for reproducible test runs. |
 185 | +| `supports_cli` | `False` | Whether the bitcoin-cli utility is compiled and available for the test. |
 186 | +| `tmpdir` | `"/var/folders/.../"` | Sets directory for test logs. Will be deleted upon a successful test run unless `nocleanup` is set to `True` |
 187 | +| `trace_rpc` | `False` | Logs all RPC calls if set to `True`. |
 188 | +| `usecli` | `False` | Uses the bitcoin-cli interface for all bitcoind commands instead of directly calling the RPC server. Requires `supports_cli`. |

MarcoFalke commented at 8:15 PM on November 4, 2019:

Instead of duplicating all help text, you could only mention the command line arg name and the TestShell name, and then tell users to refer to to the command line help string

jachiang commented at 7:42 AM on November 5, 2019:

Thank you @MarcoFalke for the review. The command line help doesn't cover all the parameters I suppose. setup_clean_chain is overridden in set_test_params() and isn't covered by cmdline help.

Would it be an acceptable solution to reduce the documentation down the parameters which don't have help text (e.g. setup_clean_chain, num_nodes, ...) , and refer to the command-line help string for the remaining params?

MarcoFalke commented at 8:15 PM on November 4, 2019: member

Thanks for working on this!

in test/functional/test-shell.md:40 in 19139ee034

  35 | +```
  36 | +
  37 | +The following `TestShell` methods manage the lifetime of the underlying bitcoind
  38 | +processes and logging utilities.
  39 | +
  40 | +* `TestShell.setup()`

instagibbs commented at 8:26 PM on November 4, 2019:

This should probably be TestShell().setup()

MarcoFalke commented at 8:28 PM on November 4, 2019:

This is not for the interactive session, but more a documentation of these member functions

jachiang commented at 7:36 AM on November 5, 2019:

Hm. @MarcoFalke agree with you, but I am thinking it may be useful for setup() to return self so we can chain it to initializer as suggested by @instagibbs:

test = TestShell().setup()

in test/functional/test-shell.md:41 in 19139ee034

  36 | +
  37 | +The following `TestShell` methods manage the lifetime of the underlying bitcoind
  38 | +processes and logging utilities.
  39 | +
  40 | +* `TestShell.setup()`
  41 | +* `TestShell.shutdown()`

instagibbs commented at 8:26 PM on November 4, 2019:

This should probably be TestShell().shutdown()

jachiang commented at 3:27 PM on November 5, 2019: contributor

Post-merge suggestions are tracked in #17378 (Sorry for the additional review effort).
Since I am not sure, I have opened an issue track doc consolidation in #17380.

MarcoFalke referenced this in commit 54a337478d on Nov 5, 2019

jasonbcox referenced this in commit 074c6b258a on Nov 3, 2020

jasonbcox referenced this in commit e80d5b9ce1 on Nov 3, 2020

deadalnix referenced this in commit 5a1a1b1bde on Nov 3, 2020

jasonbcox referenced this in commit df335a3601 on Nov 3, 2020

jasonbcox referenced this in commit 1c71a7fb0e on Nov 3, 2020

jasonbcox referenced this in commit 4e57f46856 on Nov 3, 2020

jasonbcox referenced this in commit 1926522d92 on Nov 3, 2020

MarcoFalke locked this on Dec 16, 2021