Refactor of BitcoinTestFramework main function.

[jachiang]

In order to wrap the BitcoinTestFramework class in a user-defined class, which can be started and shutdown, the code in BitcoinTestFramework main has been encapsulated in respective functions. Additionally, test-run specific state in temp-directory and NetworkThread is now reset after shutdown.

Behavioural changes:
Potential exceptions raised from any code in setup section of the code are now handled. Previously, only exceptions raised by code beginning here was handled.

TODO's:

• Refactor NetworkThread class in mininode.py, so each BitcoinTestFramework object initialises a different network thread.
• Documentation (markdown file) describing how to run an interactive test with TestWrapper.

[jnewbery]

nit: the convention seems to be that comments usually go the line above in the python test code

[jachiang]

Fixed in b1f803d03fec4cf4f66cb142ddde705b96aa8205.

[jnewbery]

The second parameter (exit) isn't used in this repo. Can you remove it by having self.shutdown return the exit_code and then main() calls sys.exit().

[jachiang]

Fixed in e163c98169e0405255a2e6acdbb679494b5fa42d.

[jnewbery]

I think it makes more logical sense to have the exception handling code in the main() function, and then just start shutdown() at the if self.success == TestStatus.FAILED and ... line

[jachiang]

I am not sure I fully understand your suggestion. If we move error handling into main, this is code that needs to be replicated in TestWrapper.

[jnewbery]

why not:

        try:
            self.setup()
            self.run_test()
        except BaseException as e:
            handle_exception(e)
        finally:
            exit_code = self.shutdown()
            sys.exit(exit_code)

[jnewbery]

my preference would be to not have this as a separate function from main()

[jachiang]

Addressed in e163c98169e0405255a2e6acdbb679494b5fa42d.

[jnewbery]

This isn't quite right. Since logging.shutdown() is called in TestFramework.shutdown(), no more logging should take place (see https://docs.python.org/3/library/logging.html#logging.shutdown).

A better way to do this would be to replace the logging.shutdown() call with:

handlers = self.log.handlers
for h in handlers:
    h.flush()
    h.flush()
    self.log.removeHandler(h)

That way, when setup is called again, the old handlers will have been removed and the new handlers will be added to the Logging object.

[jachiang]

Thanks for this suggestion. Added to e163c98169e0405255a2e6acdbb679494b5fa42.

[jnewbery]

You should also reword the second commit log to remove reference to TestWrapper if you want to get this merged to Bitcoin Core.

[jnewbery]

You should also reword the second commit log to remove reference to TestWrapper if you want to get this merged to Bitcoin Core.

Thinking about this some more, perhaps a good way to get this merged to Bitcoin Core would be to include the TestWrapper and add some documentation about how to run an interactive test. That's quite a compelling rationale for making these changes.

I'd hold off on that while we're still iterating on TestWrapper, but after the workshops let's open a PR against https://github.com/bitcoin/bitcoin.

[jachiang]

I am adding another TODO for this PR.

• Refactor NetworkThread class in mininode.py, so each BitcoinTestFramework object initialises a different network thread.

[jnewbery]

Refactor NetworkThread class in mininode.py, so each BitcoinTestFramework object initialises a different network thread.

I don't think we want more than one instance of BitcoinTestFramework to exist at a time. @jachiang - do you have a reason for wanting this?

[jachiang]

Refactor NetworkThread class in mininode.py, so each BitcoinTestFramework object initialises a different network thread.

I don't think we want more than one instance of BitcoinTestFramework to exist at a time. @jachiang - do you have a reason for wanting this?

I will omit this from this PR, because it requires limiting one NetworkThread per BitcoinTestFramework, which I am unsure of how to best implement with minimal code changes.

[jnewbery]

Do these need to be flushed before being removed?

[jnewbery]

why not:

        try:
            self.setup()
            self.run_test()
        except BaseException as e:
            handle_exception(e)
        finally:
            exit_code = self.shutdown()
            sys.exit(exit_code)

[jnewbery]

A few comments inline. I also think the first commit comment ("Removal of residual NetworkThread state.") could be expanded a bit to explain why you're making that change.

Once the documentation is added, I think this is ready to open as a PR against bitcoin/bitcoin

[jnewbery]

Is this new traceback functionality necessary? Can it be separated into a new commit so you're not moving and changing code in the same commit?

[jachiang]

Not Ideal, I agree. It seems the logger will only capture the traceback if error logging is called directly inside except (see https://stackoverflow.com/a/5191885 and https://docs.python.org/3/library/logging.html#logging.error). Since we are invoking error/exception logging elsewhere, we reconstruct the traceback string from the exception object.

[jnewbery]

pep8 import ordering please.

More generally, can you run flake8 over this new file and resolve any style issues?

[jnewbery]

is this required? num_nodes defaults to 3 in the setup() method.

[jnewbery]

I suggest you default this to 1

[jnewbery]

Have you tried changing the method signature to catch *kwargs and then set the defaults below, to avoid this huge function signature?

[jachiang]

Yes, it is much cleaner as you suggested. Thank you!

[jachiang]

@jnewbery Sorry this took a while, but I've addressed all feedback, reworked commits for easier review and added a documentation file for the TestWrapper class.

I will review again tomorrow and then open a PR against master.
A very big thank you for your time spent reviewing and improving this!

[jnewbery]

PR opened against bitcoin/bitcoin here: bitcoin#17288