Skip to content

Added util.py, includes source directory import and TestWrapper class. - #8

Merged
jnewbery merged 1 commit into
bitcoinops:masterfrom
jachiang:TestWrapper
Sep 2, 2019
Merged

jnewbery merged 1 commit into
bitcoinops:masterfrom
jachiang:TestWrapper

Conversation

@jachiang

@jachiang jachiang commented Aug 30, 2019

Copy link
Copy Markdown
Contributor

This PR moves the TestWrapper class from the bitcoin core source directory to the taproot-workhop repository. The bitcoin source directory path import has also been consolidated here.

Users now can do this:

  • Edit source directory in util.py
  • Run import util
  • Run test = util.TestWrapper()

This PR depends on:
The refactor of main in BitcoinTestFramework, which is merged into the bitcoin/optech-taproot branch. The refactor spilts out setup and shutdown from main so these methods are available to child classes, such as TestWrapper. This has been merged in optech-taproot-develop and tagged v0.1.

TODO's:

  • Implement singleton pattern during startup to ensure not other bitcoind processes are running, with a cross-platform compatible-way of acquiring PIDs.

Comment thread util.py
@@ -0,0 +1,80 @@
# Enter your source directory between the quotes here
SOURCE_DIRECTORY = ''

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im not strong on best practices for keeping user config values separate from other utility functions. Could keep as setup.py and somehow reference path from util.py, but could be awkward.

@jachiang jachiang Aug 30, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the only reason source dir path import and TestWrapper are consolidated in util.py is because I wanted to avoid to reference path from util.py, after setup.py sets source directory.

Comment thread util.py
"""Wrapper Class for BitcoinTestFramework.

Provides the BitcoinTestFramework rpc & daemon process management
functionality to external python projects."""

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might note here that the value of this class instead of just using BitcoinTestFramework is that weve provided a bunch of sane default settings as well as defaulting to setup 3 nodes. (at least that is my understanding of the raison d'etre for this class)

@jachiang jachiang Aug 30, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, BitcoinTestFramework sets defaults when reading command line arguments with argparse. Since we don't do that, we have to set the defaults in TestWrapper. Only when these options are set can we run the setup() and shutdown().

@jachiang

Copy link
Copy Markdown
Contributor Author

@jnewbery I am unsure of how best to implement the singleton pattern and detect other bitcoind instances. pidof isn't necessarily installed on OSX. Windows requires a different utility. psutil is cross-platform but requires a package install. If no obvious solutions, I suggest to hold off on this for now, since startup of a TestWrapper already fails if another instance already running.

I would like to clarify how to move forward on this TestWrapper, since all other notebook pages depend on this PR.

@jachiang jachiang mentioned this pull request Aug 31, 2019
1 task
@jnewbery

jnewbery commented Sep 2, 2019

Copy link
Copy Markdown
Contributor

I'm going to merge this now. We can do a few tidy-ups to this later, but it's important we get this in so that the individual notebooks can be built on top of it.

@jnewbery
jnewbery merged commit 2f30c7f into bitcoinops:master Sep 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants