BitcoinTestFramework instructs developers in its docstring to override
set_test_params and run_test in subclasses while being sure NOT to
override __init__ and main . This change adds a metaclass to ensure
that developers adhere to that protocol, raising a TypeError in
instances where they have not.
closes #12835
75 | + def test_exempts_framework_class(self): 76 | + class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): 77 | + pass 78 | + 79 | +if __name__ == '__main__': 80 | + unittest.main()
As is this is not integrated into your current CI. Still trying to wrap my head around everything so I could be wrong on this but since you are using your own test suite / runner which uses this meta class I don't think there's a great way to integrate into your current workflow. This can be tested directly from the command line as needed
444 | @@ -444,6 +445,9 @@ def set_test_params(self): 445 | self.num_nodes = 2 446 | self.setup_clean_chain = True 447 | 448 | + def run_test(self):
To more strictly enforce this protocol I had to add this here, even though it just calls the superclass implementation anyway. I figured this is cleaner than making an exception in the metaclass
That's fine. ComparisonTestFramework should hopefully go away very soon (#11818)
Needs rebase
Hmm have there been intermittent travis failures? I looked into both here but can't see how they would be directly related to the change. Here's a quick summary of them
26871.1
$ if [ "$CHECK_DOC" = 1 -a "$TRAVIS_EVENT_TYPE" = "pull_request" ]; then contrib/devtools/lint-all.sh; fi
src/walletinitinterface.h seems to be missing the expected include guard:
#ifndef BITCOIN_WALLETINITINTERFACE_H
#define BITCOIN_WALLETINITINTERFACE_H
...
#endif // BITCOIN_WALLETINITINTERFACE_H
^---- failure generated from contrib/devtools/lint-include-guards.sh
26871.3
File "/home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/test_framework/mininode.py", line 213, in send_message
raise IOError('Not connected, no pushbuf')
OSError: Not connected, no pushbuf
Hmm have there been intermittent travis failures?
This failure is caused by a new linter. Rebasing on master will fix this for you.
Concept ACK. I think the metaclass is useful to catch non-conforming tests. However, I think adding unit tests to test the test framework is probably overkill.
I think I'd prefer this PR if the BitcoinTestMetaClass were just added directly to test_framework.py.
Easy enough to revert. Do you want me to include the metaclass directly in test_framework.py instead of putting it in its own module?
Jup, that saves one file and one import. I'd agree to that.
Looks good. I'll ACK once commits are squashed.
Looks like the failure here was on a make job on Travis but this change shouldn't not impact any compilation - any chance to restart that?
I agree. Travis failure seems unrelated. I've restarted the job.
Travis error is unrelated. utACK, this is very neat! Learned something new.
BitcoinTestFramework instructs developers in its docstring to override
`set_test_params` and `run_test` in subclasses while being sure NOT to
override `__init__` and `main` . This change adds a metaclass to ensure
that developers adhere to that protocol, raising a ``TypeError`` in
instances where they have not.
closes #12835
Concept ACK
452 | @@ -432,6 +453,7 @@ def _initialize_chain_clean(self): 453 | for i in range(self.num_nodes): 454 | initialize_datadir(self.options.tmpdir, i) 455 | 456 | +
please remove stray newline
sure. Fine to leave it in.
Tested ACK c9cce0a7f66e5abe6a94704eb478e0dc52a29f13
There's a stray newline in your commit. Please remove!
utACK c9cce0a7f66e5abe6a94704eb478e0dc52a29f13. (Seems fine to add the new line, since it is already done.)
utACK c9cce0a