Move stop/start functions from utils.py into BitcoinTestFramework

jnewbery commented at 3:04 pm on June 8, 2017: member

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 commented at 3:07 pm on June 8, 2017: member

@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.

in test/functional/zmq_test.py:85 in 86d4529526 outdated

 97+                assert_equal(msgSequence, blockcount + 1)
 98                 blockcount += 1
 99 
100-        for x in range(0,n):
101-            assert_equal(genhashes[x], zmqHashes[x]) #blockhash from generate must be equal to the hash received over zmq
102+        for x in range(0, n):

jimmysong commented at 4:04 pm on June 8, 2017:

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

jnewbery commented at 4:41 pm on June 8, 2017:

Agree. Changed in #10555

in test/functional/zmq_test.py:17 in dbc0700b6f outdated

12 from test_framework.util import (assert_equal,
13                                  bytes_to_hex_str,
14                                  )
15 
16+def wait_for_multipart(socket, timeout=60):
17+    while True:

jimmysong commented at 4:07 pm on June 8, 2017:

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")

jnewbery commented at 4:41 pm on June 8, 2017:

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

in test/functional/fundrawtransaction.py:455 in 8f4dbd5857 outdated

451@@ -452,7 +452,7 @@ def run_test(self):
452         self.stop_node(2)
453         self.stop_node(3)
454         self.nodes[1].encryptwallet("test")
455-        bitcoind_processes[1].wait(timeout=BITCOIND_PROC_WAIT_TIMEOUT)
456+        self.bitcoind_processes[1].wait(60)

jimmysong commented at 4:11 pm on June 8, 2017:

Why take out the constant here?

jnewbery commented at 4:43 pm on June 8, 2017:

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.

MarcoFalke commented at 12:23 pm on June 18, 2017:

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

jnewbery commented at 4:06 pm on June 18, 2017:

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

MarcoFalke commented at 2:15 pm on June 19, 2017:

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

jnewbery commented at 8:26 pm on June 20, 2017:

Oops. Fixed but not pushed.

Should be good now.

in test/functional/test_framework/test_framework.py:19 in 343e137683 outdated

13@@ -14,6 +14,7 @@
14 import sys
15 import tempfile
16 import time
17+import traceback

jimmysong commented at 4:13 pm on June 8, 2017:

Is this being used?

jnewbery commented at 4:45 pm on June 8, 2017:

Here: https://github.com/bitcoin/bitcoin/blob/9c248e39f2807a7f89e555e99cc55cb2de1ad335/test/functional/test_framework/test_framework.py#L194

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

in test/functional/bip9-softforks.py:19 in c6460011f8 outdated

14@@ -15,16 +15,17 @@
15 test that enforcement has not triggered (which triggers ACTIVE)
16 test that enforcement has triggered
17 """
18+from io import BytesIO
19+import shutil

jimmysong commented at 4:15 pm on June 8, 2017:

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

jnewbery commented at 4:46 pm on June 8, 2017:

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.

jimmysong commented at 4:19 pm on June 8, 2017: contributor

utAck f7acc7adea731589aec8a7d9e16cc9069e24b5a9

Added a few nits and questions. Testing next.

jnewbery commented at 4:46 pm on June 8, 2017: member

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

jimmysong commented at 7:28 pm on June 8, 2017: contributor

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

jnewbery force-pushed on Jun 8, 2017

jnewbery commented at 7:37 pm on June 8, 2017: member

rebased on #10555

jimmysong commented at 8:17 pm on June 8, 2017: contributor

Tested ACK e1e3dd074b71503d8f74651bf2a3cfd9e55a4d2b

achow101 commented at 9:05 pm on June 8, 2017: member

utACK e1e3dd074b71503d8f74651bf2a3cfd9e55a4d2b

fanquake added the label Tests on Jun 8, 2017

in test/functional/test_framework/test_framework.py:244 in e1e3dd074b outdated

238+        try:
239+            for i in range(num_nodes):
240+                rpcs.append(self.start_node(i, dirname, extra_args[i], rpchost, timewait=timewait, binary=binary[i]))
241+        except:
242+            # If one node failed to start, stop the others
243+            self.stop_nodes()

kallewoof commented at 1:20 am on June 9, 2017:

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?

achow101 commented at 1:27 am on June 9, 2017:

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

jnewbery commented at 3:56 pm on June 9, 2017:

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?

kallewoof commented at 6:46 am on June 12, 2017:

@jnewbery Fix looks good to me!

kallewoof approved

kallewoof commented at 1:21 am on June 9, 2017: member

Tested ACK e1e3dd0

jnewbery force-pushed on Jun 9, 2017

jnewbery force-pushed on Jun 16, 2017

jnewbery commented at 1:56 pm on June 16, 2017: member

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

jnewbery force-pushed on Jun 18, 2017

jnewbery commented at 4:06 pm on June 18, 2017: member

Rebased

jnewbery force-pushed on Jun 20, 2017

laanwj commented at 12:56 pm on June 21, 2017: member

Needs rebase after #10533 (again, sorry)

in test/functional/test_framework/test_framework.py:187 in 68b56ce2a1 outdated

182@@ -187,7 +183,7 @@ def main(self):
183                 for fn in filenames:
184                     try:
185                         with open(fn, 'r') as f:
186-                            print("From" , fn, ":")
187+                            print("From", fn, ":")

MarcoFalke commented at 8:48 am on June 22, 2017:

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 force-pushed on Jun 27, 2017

jnewbery commented at 8:46 pm on June 27, 2017: member

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.

MarcoFalke assigned MarcoFalke on Jun 29, 2017

MarcoFalke commented at 9:05 am on June 29, 2017: member

Sorry, this needs rebase again. :(

[tests] remove unused imports from utils.py 37065d2ed2

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

jnewbery force-pushed on Jun 29, 2017

[tests] Move stop_node and start_node methods to BitcoinTestFramework

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.

cad967a892

[tests] move mocktime property and functions to BitcoinTestFramework 0d473c539e

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

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

05b8c081b4

jnewbery force-pushed on Jun 29, 2017

jnewbery commented at 10:59 am on June 29, 2017: member

rebased

MarcoFalke commented at 1:39 pm on June 29, 2017: member

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 05b8c081b435e87b08335e9f9b62a55fa1d48ecc, 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 notifications@github.com wrote:

rebased

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/10556#issuecomment-311933188, or mute the thread https://github.com/notifications/unsubscribe-auth/AGGmv1fsdTJNMgo7Qfy6NzuthY9Ulju5ks5sI4OMgaJpZM4N0NGy .

[tests] fix nits.

Thanks to Marco Falke.

5ba83c1d43

jnewbery commented at 2:08 pm on June 29, 2017: member

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 176c021d085f5a45bc9e038e760942aa648dd797

MarcoFalke merged this on Jun 29, 2017

MarcoFalke closed this on Jun 29, 2017

MarcoFalke referenced this in commit 65cc7aacfb on Jun 29, 2017

MarcoFalke referenced this in commit 28f788e47e on Sep 1, 2017

mempko referenced this in commit 8b8085e6fc on Sep 28, 2017

PastaPastaPasta referenced this in commit bbde15ac6e on Aug 8, 2019

PastaPastaPasta referenced this in commit a82f22e528 on Aug 12, 2019

codablock referenced this in commit 17bb230d74 on Sep 24, 2019

charlesrocket referenced this in commit 455aded9dc on Dec 14, 2019

barrystyle referenced this in commit 404a62ed5a on Jan 22, 2020

barrystyle referenced this in commit 4809d1d470 on Jan 22, 2020

MarcoFalke locked this on Sep 8, 2021

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