Tests: Add Metaclass for BitcoinTestFramework by WillAyd · Pull Request #12856 · bitcoin/bitcoin

WillAyd · 2018-04-02T06:01:58Z

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

WillAyd · 2018-04-02T06:04:04Z

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

WillAyd · 2018-04-02T06:04:59Z

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

WillAyd · 2018-04-02T15:22:53Z

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

jnewbery · 2018-04-02T15:25:25Z

Hmm have there been intermittent travis failures?

This failure is caused by a new linter. Rebasing on master will fix this for you.

jnewbery · 2018-04-02T16:20:30Z

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.

WillAyd · 2018-04-02T16:23:49Z

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?

maflcko · 2018-04-02T16:45:53Z

Jup, that saves one file and one import. I'd agree to that.

jnewbery · 2018-04-02T17:44:26Z

Looks good. I'll ACK once commits are squashed.

WillAyd · 2018-04-03T00:12:31Z

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?

jnewbery · 2018-04-03T16:10:30Z

I agree. Travis failure seems unrelated. I've restarted the job.

kallewoof · 2018-04-04T08:52:15Z

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

practicalswift · 2018-04-05T21:28:53Z

Concept ACK

jnewbery

Tested ACK c9cce0a

There's a stray newline in your commit. Please remove!

jnewbery · 2018-04-06T16:56:40Z

        for i in range(self.num_nodes):
            initialize_datadir(self.options.tmpdir, i)

+


please remove stray newline

That was intentional from our conversation in #12884. This would follow PEP8 so I figured bundle in here. OK to remove still if you want

sure. Fine to leave it in.

maflcko · 2018-04-06T17:08:13Z

utACK c9cce0a. (Seems fine to add the new line, since it is already done.)

laanwj · 2018-04-08T17:44:19Z

utACK c9cce0a

c9cce0a Tests: Add Metaclass for BitcoinTestFramework (Will Ayd) Pull request description: 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 Tree-SHA512: 5a47a7ead1f18361138cad4374747c4a8f29d25506f7b2c2a8c1c966a0b65e5ccf7317f9a078df8680fdab5d3fb71fee46a159c9f381878a3683c1e9f874abbe

Summary: c9cce0a Tests: Add Metaclass for BitcoinTestFramework (Will Ayd) Pull request description: 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 Tree-SHA512: 5a47a7ead1f18361138cad4374747c4a8f29d25506f7b2c2a8c1c966a0b65e5ccf7317f9a078df8680fdab5d3fb71fee46a159c9f381878a3683c1e9f874abbe Backport of Core [[bitcoin/bitcoin#12856 | PR12856]] Test Plan: `ninja check-functional` Reviewers: #bitcoin_abc, nakihito Reviewed By: nakihito Differential Revision: https://reviews.bitcoinabc.org/D6353

c9cce0a Tests: Add Metaclass for BitcoinTestFramework (Will Ayd) Pull request description: 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 bitcoin#12835 Tree-SHA512: 5a47a7ead1f18361138cad4374747c4a8f29d25506f7b2c2a8c1c966a0b65e5ccf7317f9a078df8680fdab5d3fb71fee46a159c9f381878a3683c1e9f874abbe

WillAyd commented Apr 2, 2018

View reviewed changes

fanquake added the Tests label Apr 2, 2018

fanquake requested a review from jnewbery April 2, 2018 07:03

jnewbery reviewed Apr 6, 2018

View reviewed changes

laanwj merged commit c9cce0a into bitcoin:master Apr 8, 2018

WillAyd deleted the meta-tst branch April 8, 2018 18:31

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

		for i in range(self.num_nodes):
		initialize_datadir(self.options.tmpdir, i)

Conversation

WillAyd commented Apr 2, 2018

Uh oh!

WillAyd Apr 2, 2018

Choose a reason for hiding this comment

Uh oh!

WillAyd Apr 2, 2018

Choose a reason for hiding this comment

Uh oh!

jnewbery Apr 2, 2018

Choose a reason for hiding this comment

Uh oh!

maflcko Apr 5, 2018

Choose a reason for hiding this comment

Uh oh!

WillAyd commented Apr 2, 2018

Uh oh!

jnewbery commented Apr 2, 2018

Uh oh!

jnewbery commented Apr 2, 2018

Uh oh!

WillAyd commented Apr 2, 2018

Uh oh!

maflcko commented Apr 2, 2018

Uh oh!

jnewbery commented Apr 2, 2018

Uh oh!

WillAyd commented Apr 3, 2018

Uh oh!

jnewbery commented Apr 3, 2018

Uh oh!

kallewoof commented Apr 4, 2018

Uh oh!

practicalswift commented Apr 5, 2018

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery Apr 6, 2018

Choose a reason for hiding this comment

Uh oh!

WillAyd Apr 6, 2018

Choose a reason for hiding this comment

Uh oh!

jnewbery Apr 6, 2018

Choose a reason for hiding this comment

Uh oh!

maflcko commented Apr 6, 2018

Uh oh!

laanwj commented Apr 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants