Added TestShell class for interactive Python environments. #17288
Conversation
doc/test-wrapper.md
Outdated
| @@ -0,0 +1,123 @@ | |||
| Test Wrapper for Interactive Environments | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
...or possibly in test/functional/
|
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 |
|
Concept ACK, seems like a great feature. |
|
Concept ACK: very nice idea and what an excellent first-time contribution! Hope to see more contributions from you in the future! |
There was a problem hiding this comment.
ACK 9535d81 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
doc/test-wrapper.md
Outdated
| @@ -0,0 +1,123 @@ | |||
| Test Wrapper for Interactive Environments | |||
There was a problem hiding this comment.
...or possibly in test/functional/
doc/test-wrapper.md
Outdated
|
|
||
| This document describes the usage of the `TestWrapper` submodule in the functional test suite. | ||
|
|
||
| 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: |
There was a problem hiding this comment.
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.
doc/test-wrapper.md
Outdated
|
|
||
| * Manage regtest bitcoind subprocesses. | ||
| * Access RPC interfaces of the underlying bitcoind instances. | ||
| * Log events to functional the test logging utility. |
There was a problem hiding this comment.
s/functional the/the functional/
doc/test-wrapper.md
Outdated
| * Access RPC interfaces of the underlying bitcoind instances. | ||
| * Log events to functional the test logging utility. | ||
|
|
||
| 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, |
There was a problem hiding this comment.
nit: s/command-line/command line/
doc/test-wrapper.md
Outdated
| * Access RPC interfaces of the underlying bitcoind instances. | ||
| * Log events to functional the test logging utility. | ||
|
|
||
| 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, |
| @@ -135,6 +163,9 @@ def main(self): | |||
| self.add_options(parser) | |||
| self.options = parser.parse_args() | |||
|
|
|||
| def setup(self): | |||
| """Call this method to startup the test-framework object with options set.""" | |||
There was a problem hiding this comment.
nit: s/startup/start up/ and s/test-framework/test framework/
|
|
||
| if success == TestStatus.FAILED and self.options.pdbonfailure: | ||
| def shutdown(self): | ||
| """Call this method to shutdown the test-framework object.""" |
There was a problem hiding this comment.
nit: s/shutdown/shut down/ and s/test-framework/test framework/
| class TestWrapper: | ||
|
|
||
| class __TestWrapper(BitcoinTestFramework): | ||
|
|
There was a problem hiding this comment.
nit: possibly remove the extra lines at L21, 23 and 31
| print("TestWrapper is already running!") | ||
| return | ||
|
|
||
| self.setup_clean_chain = kwargs.get('setup_clean_chain',True) |
There was a problem hiding this comment.
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.
doc/test-wrapper.md
Outdated
|
|
||
| The following utility consolidates logs from the bitcoind nodes and the underlying BitcoinTestFramework: | ||
|
|
||
| * `/path/to/bitcoin/test/functional/combine_logs.py '/path/to/bitcoin_func_test_XXXXXXX'` |
There was a problem hiding this comment.
nit: missing newline at EOF
|
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 |
I think the name is ok. Other options: |
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/ |
There was a problem hiding this comment.
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.
| self.log.info("Test skipped") | ||
| exit_code = TEST_EXIT_SKIPPED | ||
| else: | ||
| self.log.error("Test failed. Test logging available at %s/test_framework.log", self.options.tmpdir) | ||
| 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)) | ||
| exit_code = TEST_EXIT_FAILED | ||
| logging.shutdown() | ||
| for h in list(self.log.handlers): |
There was a problem hiding this comment.
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.
| # file COPYING or http://www.opensource.org/licenses/mit-license.php. | ||
| """Wrapper Class for BitcoinTestFramework. | ||
|
|
||
| The TestWrapper class extends the BitcoinTestFramework |
There was a problem hiding this comment.
I'd move this to be a class docstring for TestWrapper rather than a file docstring.
| @@ -0,0 +1,83 @@ | |||
| #!/usr/bin/env python3 | |||
| # Copyright (c) 2014-2019 The Bitcoin Core developers | |||
There was a problem hiding this comment.
nit: (c) 2019 (unless you've been secretly working on this for 5 years)
There was a problem hiding this comment.
Almost feels like it ;)
|
|
||
| instance = None | ||
|
|
||
| def __new__(cls): |
There was a problem hiding this comment.
Small comment here to explain that we wrap the class to enforce a singleton pattern would be nice.
| def setup(self, **kwargs): | ||
|
|
||
| if self.running: | ||
| print("TestWrapper is already running!") |
There was a problem hiding this comment.
Make any sense to log instead of print? (same question for below)
There was a problem hiding this comment.
Thanks @jamesob for the review. For my understanding, do you mean logging it to the (already) running TestShell/Driver instance?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
|
Concept ACK |
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?) |
|
Since we're discussing names, can offer up |
|
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
the
2019-10-testwrapper
branch
3 times, most recently
from
401eb96
to
74b5fac
Compare
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.
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.
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.
TestNode objects need to be removed during shutdown, as setup_nodes does not remove previous TestNode objects from previous test runs during setup.
This allows a BitcoinTestFramework child class to set test parameters in an overridden setup() rather than in an overridden set_test_params().
This ensures TestFramework default parameters are set before setup is called. A child class will therefore have access to defaults when overriding setup.
jachiang
force-pushed
the
2019-10-testwrapper
branch
from
74b5fac
to
e521ff6
Compare
jachiang
force-pushed
the
2019-10-testwrapper
branch
from
e521ff6
to
d42616a
Compare
test/functional/README.md
Outdated
| #### Prototyping tests | ||
|
|
||
| The [`TestShell`](test-shell.md) class exposes the BitcoinTestFramework | ||
| functionality to interactive Python3 environments, and can be used to prototype |
There was a problem hiding this comment.
nit: remove comma after environments, as the clause after is not independent.
test/functional/test-shell.md
Outdated
| Test Shell for Interactive Environments | ||
| ========================================= | ||
|
|
||
| This document describes the usage of the `TestShell` submodule in the functional |
There was a problem hiding this comment.
nit: "describes how to use" might be friendlier.
| * Access RPC interfaces of the underlying bitcoind instances. | ||
| * Log events to the functional test logging utility. | ||
|
|
||
| The `TestShell` can be useful in interactive environments where it is necessary |
There was a problem hiding this comment.
nit: you write TestShell without backticks above and below; ideally settle on one style. Idem for BitcoinTestFramework.
test/functional/test-shell.md
Outdated
| 20XX-XX-XXTXX:XX:XX.XXXXXXX TestFramework (INFO): Initializing test directory /path/to/bitcoin_func_test_XXXXXXX | ||
| ``` | ||
| The TestShell forwards all functional test parameters of the parent Bitcoin | ||
| TestFramework object. The full set of argument keywords which can be used to |
There was a problem hiding this comment.
nit: wrote BitcoinTestFramework elsewhere without spaces
test/functional/test-shell.md
Outdated
| **Example: Mining a regtest chain** | ||
|
|
||
| By default, the TestShell nodes are initialized with a clean chain. This means | ||
| that each node of the TestShell is initialized with a block height of 0. |
There was a problem hiding this comment.
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"]
200Second 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"]
200There was a problem hiding this comment.
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!!
There was a problem hiding this comment.
Fixed in documentation:test.setup(num_nodes=2, setup_clean_chain=True).
|
|
||
| ``` | ||
| >>> test.nodes[0].log.info("Successfully mined regtest chain!") | ||
| 20XX-XX-XXTXX:XX:XX.XXXXXXX TestFramework.node0 (INFO): Successfully mined regtest chain! |
| ``` | ||
| >>> test2 = TestShell() | ||
| >>> test2.setup() | ||
| TestShell is already running! |
| **Note:** `TestShell.reset()` will reset test parameters to default values and | ||
| can be called after the TestShell is shut down. | ||
|
|
||
| | Test parameter key | Default Value | Description | |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
BitcoinTestFrameworkcommand-line args vsBitcoinTestFrameworkmember:--tracerpcvstrace_rpc--portseedvsport_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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
jachiang
force-pushed
the
2019-10-testwrapper
branch
from
d42616a
to
19139ee
Compare
| # Num_nodes parameter must be set | ||
| # by BitcoinTestFramework child class. | ||
| self.num_nodes = kwargs.get('num_nodes', 1) | ||
| kwargs.pop('num_nodes', None) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Much better. Fixed. Thank you!
| **Note:** `TestShell.reset()` will reset test parameters to default values and | ||
| can be called after the TestShell is shut down. | ||
|
|
||
| | Test parameter key | Default Value | Description | |
There was a problem hiding this comment.
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.
|
ACK 19139ee |
|
ACK 19139ee Verified that setting up with the updated instruction of |
jachiang
force-pushed
the
2019-10-testwrapper
branch
from
19139ee
to
a630165
Compare
|
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. |
jachiang
force-pushed
the
2019-10-testwrapper
branch
from
a630165
to
19139ee
Compare
19139ee Add documentation for test_shell submodule (JamesC) f511236 Add TestShell class (James Chiang) 5155602 Move argparse() to init() (JamesC) 2ab0146 Move assert num_nodes is set into main() (JamesC) 614c645 Clear TestNode objects after shutdown (JamesC) 6f40820 Add closing and flushing of logging handlers (JamesC) 6b71241 Refactor TestFramework main() into setup/shutdown (JamesC) ede8b76 Remove network_event_loop instance in close() (JamesC) Pull request description: 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](https://github.com/bitcoinops/taproot-workshop) 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` ACKs for top commit: jamesob: ACK 19139ee jonatack: ACK 19139ee jnewbery: 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. jachiang: > Rather than invalidate the three ACKs for a minor nit, can you force push back to [19139ee](19139ee) please? I think this PR was ready to merge before your last force push. jnewbery: ACK 19139ee Tree-SHA512: 0c24f405f295a8580a9c8f1b9e0182b5d753eb08cc331424616dd50a062fb773d3719db4d08943365b1f42ccb965cc363b4bcc5beae27ac90b3460b349ed46b2
|
Could do the fixup as a follow-up? |
|
very nice! post-hom concept ACK |
|
|
||
| def reset(self): | ||
| if self.running: | ||
| print("Shutdown TestWrapper before resetting!") |
There was a problem hiding this comment.
| print("Shutdown TestWrapper before resetting!") | |
| print("Shutdown TestShell before resetting!") |
There was a problem hiding this comment.
Thank you. I'll follow-up.
| ``` | ||
| >>> import sys | ||
| >>> sys.path.insert(0, "/path/to/bitcoin/test/functional") | ||
| >>> from test_framework.test_shell import `TestShell` |
There was a problem hiding this comment.
Sorry. I'll follow-up with a fix.
| | `supports_cli` | `False` | Whether the bitcoin-cli utility is compiled and available for the test. | | ||
| | `tmpdir` | `"/var/folders/.../"` | Sets directory for test logs. Will be deleted upon a successful test run unless `nocleanup` is set to `True` | | ||
| | `trace_rpc` | `False` | Logs all RPC calls if set to `True`. | | ||
| | `usecli` | `False` | Uses the bitcoin-cli interface for all bitcoind commands instead of directly calling the RPC server. Requires `supports_cli`. | |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
| The following `TestShell` methods manage the lifetime of the underlying bitcoind | ||
| processes and logging utilities. | ||
|
|
||
| * `TestShell.setup()` |
There was a problem hiding this comment.
This should probably be TestShell().setup()
There was a problem hiding this comment.
This is not for the interactive session, but more a documentation of these member functions
There was a problem hiding this comment.
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()
| processes and logging utilities. | ||
|
|
||
| * `TestShell.setup()` | ||
| * `TestShell.shutdown()` |
There was a problem hiding this comment.
This should probably be TestShell().shutdown()
|
2493770 TestShell: Return self from setup() (James Chiang) a8dea45 TestShell: Simplify default setting of num_nodes (James Chiang) 9c7806e Doc: Remove backticks in test-shell.md code block (James Chiang) d3ed06e TestShell: Fix typo in TestShell warning printout (James Chiang) Pull request description: This PR follows up on #17288 and fixes typos and implements code clean-ups suggested by reviewers of 19139ee. - Typo in `test_shell.py` warning - Typo in `test-shell.md` code block - Simplified default setting of `num_nodes` in `TestShell.setup()` - Enable initializer chaining: `TestShell().setup()` ACKs for top commit: MarcoFalke: ACK 2493770 instagibbs: tACK 2493770 jnewbery: utACK 2493770 Tree-SHA512: 8fa7c2c550dbc3ec899de9dc328cd55cfa6daafe3b888aa5427e72fea69f064d938ec68e15bfa57109c0f6c3583e627ac4bd69303a11575d056941bd253adee0
Summary: > 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. This is a backport of Core [[bitcoin/bitcoin#17288 | PR17288]] [1/7] bitcoin/bitcoin@ede8b76 Test Plan: `ninja && ninja check-functional` Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D8241
Summary: > 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. In our codebase, `set_test_params()` is called in `main()` rather than in `__init__` because we want parser options to be available during test setup (D1974). This is a backport of Core [[bitcoin/bitcoin#17288 | PR17288]] [2/7] bitcoin/bitcoin@6b71241 Depends on D8241 Test Plan: `ninja && ninja check-functional` Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D8242
Summary: > 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. This is a backport of Core [[bitcoin/bitcoin#17288 | PR17288]] [3/8] bitcoin/bitcoin@6f40820 Depends on D8242 Test Plan: `ninja && ninja check` Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D8243
Summary: > TestNode objects need to be removed during shutdown, as setup_nodes does not remove previous TestNode objects from previous test runs during setup. This is a backport of Core [[bitcoin/bitcoin#17288 | PR17288]] [4/7] bitcoin/bitcoin@614c645 Depends on D8243 Test Plan: `ninja && ninja check` Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D8244
Summary: > This ensures TestFramework default parameters are set before setup is called. > A child class will therefore have access to defaults when overriding setup. In our codebase, this was already achieved by the change in D1974 which move d`set_test_params` into `main()` after parsing arguments. This commit makes our codebase more similar to Core, while keeping our ordering of method calls: `parse_args` before `set_test_params`. This is a backport of Core [[bitcoin/bitcoin#17288 | PR17288]] [5/7] It is inspired by the following 2 commits while taking into account changes introduced in D1974: - bitcoin/bitcoin@2ab0146 - bitcoin/bitcoin@5155602 Depends on D8244 Test Plan: `ninja && ninja check` Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D8245
Summary: > 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. This is a backport of Core [[bitcoin/bitcoin#17288 | PR17288]] [6/7] bitcoin/bitcoin@f511236 Depends on D8245 Test Plan: ``` $ python3 >>> import sys >>> sys.path.insert(0, "/home/pierre/dev/bitcoin-abc/test/functional") >>> from test_framework.test_shell import TestShell >>> test = TestShell() >>> test.setup(num_nodes=2, configfile="/home/pierre/dev/bitcoin-abc/build/test/config.ini") 2020-11-03T09:36:41.939000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_cu4wq1lu >>> test.nodes[0].generate(2) ['1239cf08ec1a14d8fd058a6ce7d01bfca7b72ba2cafe98a2d792b545d3b47031', '20b4a2dad83310cad98f1db91000267bb33e8d927a59ade6f371200de55ae216'] >>> test.nodes[0].getblockchaininfo() {'chain': 'regtest', 'blocks': 202, 'headers': 202, 'bestblockhash': '20b4a2dad83310cad98f1db91000267bb33e8d927a59ade6f371200de55ae216', 'difficulty': Decimal('4.656542373906925E-10'), 'mediantime': 1296688637, 'verificationprogress': 1, 'initialblockdownload': False, 'chainwork': '0000000000000000000000000000000000000000000000000000000000000196', 'size_on_disk': 46753, 'pruned': False, 'softforks': {'testdummy': {'type': 'bip9', 'bip9': {'status': 'started', 'bit': 28, 'start_time': 0, 'timeout': 9223372036854775807, 'since': 144, 'statistics': {'period': 144, 'threshold': 108, 'elapsed': 59, 'count': 59, 'possible': True}}, 'active': False}}, 'warnings': 'This is a pre-release test build - use at your own risk - do not use for mining or merchant applications'} >>> test.shutdown() 2020-11-03T09:37:44.840000Z TestFramework (INFO): Stopping nodes 2020-11-03T09:37:45.097000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_cu4wq1lu on exit 2020-11-03T09:37:45.097000Z TestFramework (INFO): Tests successful ``` Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D8246
Summary: Documentation-only change. This is a backport of Core [[bitcoin/bitcoin#17288 | PR17288]] [7/7] bitcoin/bitcoin@19139ee It includes a fix from [[bitcoin/bitcoin#17378 | PR17378]]: bitcoin/bitcoin@9c7806e Depends on D8246 Test Plan: Proof-reading Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D8247
This PR refactors BitcoinTestFramework to encapsulate setup and shutdown logic into dedicated methods, and adds a
TestWrapperTestShell 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
TestWrapperTestShell is motivated by the opportunity to expose the test-framework as a prototyping and educational toolkit. Examples of code prototypes enabled byTestWrapperTestShell can be found in the Optech Taproot/Schnorr workshop repository.Usage example:
Overview of changes to BitcoinTestFramework:
setup()/shutdown()methods.parse_argsmethod.BitcoinTestFramework.success.During Shutdown
BitcoinTestFrameworklogging handlers are flushed and removed.BitcoinTestFrameowork.nodeslist is cleared.NetworkThread.network_event_loopis reset. (NetworkThread class).Behavioural changes:
set_test_paramsmethod.Added files:
test_wrapper.pytest_shell.pytest-wrapper.mdtest-shell.md