Move stop/start functions from utils.py into BitcoinTestFramework #10556

jnewbery · 2017-06-08T15:04:31Z

Final preparation step in #10082 before introducing the TestNode class.

This builds on top of #10555. The first 5 commits are from that PR. Without those commits, the travis build times out.

This PR moves the stop/start functions from utils.py into BitcoinTestFramework. It also moves stateful functions/variables from utils.py into BitcoinTestFramework (coverage variables, bitcoind_processes dict, mocktime functions). It also does some general tidyup of test_framework.py and utils.py.

This touches a few individual test cases, but those changes are very minor. The important changes are in util.py and test_framework.py. The final commit is a code move only.

jnewbery · 2017-06-08T15:07:04Z

@jimmysong @kallewoof @sdaftuar - you've all mentioned that you're happy to help move #10082 forward. Any review of this and/or #10555 would be appreciated.

jimmysong

utAck f7acc7adea731589aec8a7d9e16cc9069e24b5a9

Added a few nits and questions. Testing next.

jimmysong · 2017-06-08T16:04:08Z

test/functional/zmq_test.py

nit: range(n) is better than range(0, n)

Agree. Changed in #10555

jimmysong · 2017-06-08T16:07:58Z

test/functional/zmq_test.py

This might be easier to read if made into a for loop:

interval = 0.1 for _ in range(int(timeout/interval)): try: return socket.recv_multipart(zmq.NOBLOCK) except zmq.error.Again: time.sleep(interval) raise AssertionError("Timed out waiting for zmq message")

Yes, that's better. I've made the change in #10555, and will rebase this PR off that once you've finished your review.

jimmysong · 2017-06-08T16:11:11Z

test/functional/fundrawtransaction.py

Why take out the constant here?

Because I've moved the BITCOIND_PROC_WAIT_TIMEOUT constant into test_framework.py. The next PR (introduce TestNode) will remove bitcoind_processes dict entirely, so for this intermediate PR rather than add an import which I'll revert in the next PR, I've just changed this to a hard-coded value.

jimmysong · 2017-06-08T16:13:30Z

test/functional/test_framework/test_framework.py

Is this being used?

Here:

bitcoin/test/functional/test_framework/test_framework.py

Line 194 in 9c248e3

traceback.print_exc()

I must have missed out this import in a previous commit. Makes sense to fix it in this cleanup commit.

jimmysong · 2017-06-08T16:15:22Z

test/functional/bip9-softforks.py

How did this work before? Was it one of the import *'s?

Yes, this was being indirectly imported from utils.py :( there are still lots of indirect imports in functional tests, which makes refactors like this seem more disruptive than they actually are.

jnewbery · 2017-06-08T16:46:52Z

@jimmysong - Thanks for the quick review! Responses inline.

jimmysong · 2017-06-08T19:28:00Z

Great, please let me know when you rebase to #10555 and I'll test it.

jnewbery · 2017-06-08T19:37:09Z

rebased on #10555

jimmysong · 2017-06-08T20:17:51Z

Tested ACK e1e3dd074b71503d8f74651bf2a3cfd9e55a4d2b

achow101 · 2017-06-08T21:05:51Z

utACK e1e3dd074b71503d8f74651bf2a3cfd9e55a4d2b

kallewoof

Tested ACK e1e3dd0

kallewoof · 2017-06-09T01:20:12Z

test/functional/test_framework/test_framework.py

I have a hard time understanding how this will work. self.nodes seems to be set to self.start_nodes(...) which means at this point, self.nodes is not set at all, and self.stop_nodes() will do nothing. Am I missing something?

I don't think you are. Instead of returning rpcs, I think this function should appent to self.nodes directly.

Good spot @kallewoof - you're not missing anything at all. This is obviously wrong :(

Once #10082 is fully merged, I'd like to improve the way the TestNode objects are handled by the BitcoinTestFramework class. There should be separate methods add_nodes(), which would instantiate new TestNode objects, and start_node(), which would start the bitcoind process. I don't think start_node() or start_nodes() should be responsible for updating the self.nodes list, and individual tests shouldn't normally have to concern themselves with updating the list.

I also think that eventually self.nodes should be a dictionary.

For now, I've added a fixup commit that appends the newly created nodes to self.nodes, then calls self.stop_nodes(), and then clears the list. It's a little hacky, but it should restore the functionality here until #10082 is fully merged. Can you take a look and let me know what you think?

@jnewbery Fix looks good to me!

jnewbery · 2017-06-16T13:56:01Z

Needed rebase because the new wallet-encryption test used functions that are moved into the BitcoinTestFramework class by the PR.

maflcko · 2017-06-18T12:23:55Z

test/functional/fundrawtransaction.py

Please keep the arg named. I'd really prefer if all primitive types are named in our python code.

Done. I've also restored the use of a constant here (at least until the next PR in this series is merged).

Are you sure this was fixed? GitHub didn't collapse the comments.

Oops. Fixed but not pushed.

Should be good now.

jnewbery · 2017-06-18T16:06:24Z

Rebased

laanwj · 2017-06-21T12:56:36Z

Needs rebase after #10533 (again, sorry)

maflcko · 2017-06-22T08:48:43Z

test/functional/test_framework/test_framework.py

Not sure how to feel about the whitespace changes. This might discourage or distract reviewers. Maybe we could make those in a separate somewhat larger pull?

jnewbery · 2017-06-27T20:46:12Z

rebased (twice - since master changed while I was rebasing 🙁 )

Not sure how to feel about the whitespace changes. This might discourage or distract reviewers.

I include style fixes for the modules I'm touching in PRs for a few reasons:

I like my commits to run clean through a linter. It's the best way I know to make sure I'm not introducing new style nits.
no-one likes reviewing style-change-only PRs, so unless they're included in larger PRs, style tends to degrade over time
it's less disruptive than project-wide style PRs in terms of rebasing, since the module is being updated anyway.

It doesn't appear that this has discouraged reviewers - this PR has three ACKs and a comment from another reviewer (you).

The style changes are contained within separate commits so if you feel really strongly about this I can remove them. However, I'd prefer not to since I've already rebased this PR five times.

maflcko · 2017-06-29T09:05:35Z

Sorry, this needs rebase again. :(

This commit moves functions start_node, start_nodes, stop_node and stop_nodes functions into the BitcoinTestFramework class. It also moves the bitcoind_processes dict and coverage variables into BitcoinTestFramework.

This commit re-organizes the utils.py module into logical sections.

jnewbery · 2017-06-29T10:59:07Z

rebased

maflcko · 2017-06-29T13:39:34Z

Well, the issue with including whitespace fixes is that it makes it harder to backport and also harder to keep mergeable. So the rebases will introduce errors (see below). Actually I would be fine with a cleanup in test that fixes whitespace and is a `git diff --ignore-all-space` empty diff. utACK with 05b8c08, lets leave this as is and fix two nits: * rpc_auth_pair does not exist previously and is never used, please remove * You want to move the `assert_equal(return_code, 0)` down by one line, such that the process is deleted even if the return code does not match * wait_for_node_exit should probably come with a default value for timeout, but this should be done i a follow a later pull.

…

On Thu, Jun 29, 2017 at 12:59 PM, John Newbery ***@***.***> wrote: rebased — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#10556 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGGmv1fsdTJNMgo7Qfy6NzuthY9Ulju5ks5sI4OMgaJpZM4N0NGy> .

Thanks to Marco Falke.

jnewbery · 2017-06-29T14:08:17Z

Fixed the first two nits. Thanks for catching

wait_for_node_exit should probably come with a default value for timeout,
but this should be done i a follow a later pull.

Right - I think that's completely orthogonal to this pull. It was introduced in 176c021

…tFramework 5ba83c1 [tests] fix nits. (John Newbery) 05b8c08 [tests] reorganize utils.py module (code move only) (John Newbery) 0d473c5 [tests] move mocktime property and functions to BitcoinTestFramework (John Newbery) cad967a [tests] Move stop_node and start_node methods to BitcoinTestFramework (John Newbery) f1fe536 [tests] fix flake8 warnings in test_framework.py and util.py (John Newbery) 37065d2 [tests] remove unused imports from utils.py (John Newbery) Tree-SHA512: 461db412c57c4d0030e27fe3f78f17bcaf00b966f319a9e613460cca897508ff70a29db7138133fe1be8d447dad6702ba2778f9eddfe929016e560d71c20b09f

@kallewoof

7148b74 [tests] Functional tests must explicitly set num_nodes (John Newbery) 5448a14 [tests] don't override __init__() in individual tests (John Newbery) 6cf094a [tests] Avoid passing around member variables in test_framework (John Newbery) 36b6268 [tests] TestNode: separate add_node from start_node (John Newbery) be2a2ab [tests] fix - use rpc_timeout as rpc timeout (John Newbery) Pull request description: Some additional tidyups after the introduction of TestNode: - commit 1 makes TestNode use the correct rpc timeout. This should have been included in #11077 - commit 2 separates `add_node()` from `start_node()` as originally discussed here: #10556 (comment) with @kallewoof . The test writer no longer needs to assign to `self.nodes` when starting/stopping nodes. - commit 3 adds a `set_test_params()` method, so individual tests don't need to override `__init__()` and call `super().__init__()` Tree-SHA512: 0adb030623b96675b5c29e2890ce99ccd837ed05f721d0c91b35378c5ac01b6658174aac12f1f77402e1d38b61f39b3c43b4df85c96952565dde1cda05b0db84

@kallewoof

7148b74 [tests] Functional tests must explicitly set num_nodes (John Newbery) 5448a14 [tests] don't override __init__() in individual tests (John Newbery) 6cf094a [tests] Avoid passing around member variables in test_framework (John Newbery) 36b6268 [tests] TestNode: separate add_node from start_node (John Newbery) be2a2ab [tests] fix - use rpc_timeout as rpc timeout (John Newbery) Pull request description: Some additional tidyups after the introduction of TestNode: - commit 1 makes TestNode use the correct rpc timeout. This should have been included in #11077 - commit 2 separates `add_node()` from `start_node()` as originally discussed here: bitcoin/bitcoin#10556 (comment) with @kallewoof . The test writer no longer needs to assign to `self.nodes` when starting/stopping nodes. - commit 3 adds a `set_test_params()` method, so individual tests don't need to override `__init__()` and call `super().__init__()` Tree-SHA512: 0adb030623b96675b5c29e2890ce99ccd837ed05f721d0c91b35378c5ac01b6658174aac12f1f77402e1d38b61f39b3c43b4df85c96952565dde1cda05b0db84

…coinTestFramework 5ba83c1 [tests] fix nits. (John Newbery) 05b8c08 [tests] reorganize utils.py module (code move only) (John Newbery) 0d473c5 [tests] move mocktime property and functions to BitcoinTestFramework (John Newbery) cad967a [tests] Move stop_node and start_node methods to BitcoinTestFramework (John Newbery) f1fe536 [tests] fix flake8 warnings in test_framework.py and util.py (John Newbery) 37065d2 [tests] remove unused imports from utils.py (John Newbery) Tree-SHA512: 461db412c57c4d0030e27fe3f78f17bcaf00b966f319a9e613460cca897508ff70a29db7138133fe1be8d447dad6702ba2778f9eddfe929016e560d71c20b09f

@kallewoof

7148b74 [tests] Functional tests must explicitly set num_nodes (John Newbery) 5448a14 [tests] don't override __init__() in individual tests (John Newbery) 6cf094a [tests] Avoid passing around member variables in test_framework (John Newbery) 36b6268 [tests] TestNode: separate add_node from start_node (John Newbery) be2a2ab [tests] fix - use rpc_timeout as rpc timeout (John Newbery) Pull request description: Some additional tidyups after the introduction of TestNode: - commit 1 makes TestNode use the correct rpc timeout. This should have been included in bitcoin#11077 - commit 2 separates `add_node()` from `start_node()` as originally discussed here: bitcoin#10556 (comment) with @kallewoof . The test writer no longer needs to assign to `self.nodes` when starting/stopping nodes. - commit 3 adds a `set_test_params()` method, so individual tests don't need to override `__init__()` and call `super().__init__()` Tree-SHA512: 0adb030623b96675b5c29e2890ce99ccd837ed05f721d0c91b35378c5ac01b6658174aac12f1f77402e1d38b61f39b3c43b4df85c96952565dde1cda05b0db84

@kallewoof

7148b74dc [tests] Functional tests must explicitly set num_nodes (John Newbery) 5448a1471 [tests] don't override __init__() in individual tests (John Newbery) 6cf094a02 [tests] Avoid passing around member variables in test_framework (John Newbery) 36b626867 [tests] TestNode: separate add_node from start_node (John Newbery) be2a2ab6a [tests] fix - use rpc_timeout as rpc timeout (John Newbery) Pull request description: Some additional tidyups after the introduction of TestNode: - commit 1 makes TestNode use the correct rpc timeout. This should have been included in #11077 - commit 2 separates `add_node()` from `start_node()` as originally discussed here: bitcoin/bitcoin#10556 (comment) with @kallewoof . The test writer no longer needs to assign to `self.nodes` when starting/stopping nodes. - commit 3 adds a `set_test_params()` method, so individual tests don't need to override `__init__()` and call `super().__init__()` Tree-SHA512: 0adb030623b96675b5c29e2890ce99ccd837ed05f721d0c91b35378c5ac01b6658174aac12f1f77402e1d38b61f39b3c43b4df85c96952565dde1cda05b0db84

jimmysong reviewed Jun 8, 2017

View reviewed changes

jnewbery force-pushed the testframeworkstopstart branch from f7acc7a to e1e3dd0 Compare June 8, 2017 19:29

fanquake added the Tests label Jun 8, 2017

kallewoof approved these changes Jun 9, 2017

View reviewed changes

jnewbery force-pushed the testframeworkstopstart branch from e1e3dd0 to b59e8a1 Compare June 9, 2017 15:56

jnewbery mentioned this pull request Jun 14, 2017

[tests] make pruning.py faster #10591

Closed

jnewbery force-pushed the testframeworkstopstart branch from b59e8a1 to 3cc81f9 Compare June 16, 2017 13:55

maflcko reviewed Jun 18, 2017

View reviewed changes

jnewbery force-pushed the testframeworkstopstart branch from 3cc81f9 to 1af1666 Compare June 18, 2017 15:57

jnewbery mentioned this pull request Jun 18, 2017

[tests] Demostration/concept: Add a TestNode class to test_Framework.py #10082

Closed

6 tasks

jnewbery force-pushed the testframeworkstopstart branch from 1af1666 to 68b56ce Compare June 20, 2017 20:25

maflcko reviewed Jun 22, 2017

View reviewed changes

jnewbery force-pushed the testframeworkstopstart branch 2 times, most recently from 278f4e8 to 65bf2b4 Compare June 27, 2017 20:36

maflcko self-assigned this Jun 29, 2017

jnewbery added 2 commits June 29, 2017 11:16

[tests] remove unused imports from utils.py

37065d2

[tests] fix flake8 warnings in test_framework.py and util.py

f1fe536

jnewbery force-pushed the testframeworkstopstart branch from 65bf2b4 to 5578642 Compare June 29, 2017 10:21

[tests] Move stop_node and start_node methods to BitcoinTestFramework

cad967a

This commit moves functions start_node, start_nodes, stop_node and stop_nodes functions into the BitcoinTestFramework class. It also moves the bitcoind_processes dict and coverage variables into BitcoinTestFramework.

jnewbery added 2 commits June 29, 2017 11:55

[tests] move mocktime property and functions to BitcoinTestFramework

0d473c5

[tests] reorganize utils.py module (code move only)

05b8c08

This commit re-organizes the utils.py module into logical sections.

jnewbery force-pushed the testframeworkstopstart branch from 5578642 to 05b8c08 Compare June 29, 2017 10:58

[tests] fix nits.

5ba83c1

Thanks to Marco Falke.

maflcko merged commit 5ba83c1 into bitcoin:master Jun 29, 2017

jnewbery mentioned this pull request Aug 23, 2017

TestNode tidyups #11121

Merged

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

Move stop/start functions from utils.py into BitcoinTestFramework #10556

Move stop/start functions from utils.py into BitcoinTestFramework #10556

Uh oh!

Conversation

jnewbery commented Jun 8, 2017

Uh oh!

jnewbery commented Jun 8, 2017

Uh oh!

jimmysong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Jun 8, 2017

Uh oh!

jimmysong commented Jun 8, 2017

Uh oh!

jnewbery commented Jun 8, 2017

Uh oh!

jimmysong commented Jun 8, 2017

Uh oh!

achow101 commented Jun 8, 2017

Uh oh!

kallewoof left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Jun 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Jun 18, 2017

Uh oh!

laanwj commented Jun 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Jun 27, 2017

Uh oh!

maflcko commented Jun 29, 2017

Uh oh!

jnewbery commented Jun 29, 2017

Uh oh!

maflcko commented Jun 29, 2017 via email

Uh oh!

jnewbery commented Jun 29, 2017

Uh oh!

kallewoof left a comment •

edited

Loading

laanwj commented Jun 21, 2017 •

edited

Loading