-blocknotify
and -walletnotify
notifications. The -alertnotify
test file forknotify.py
is renamed to notifications.py
to accommodate the new tests. Credits to @jnewbery for this cleanup and unification.
-blocknotify
and -walletnotify
notifications. The -alertnotify
test file forknotify.py
is renamed to notifications.py
to accommodate the new tests. Credits to @jnewbery for this cleanup and unification.
27+ self.extra_args = [['-blocknotify=echo %%s >> %s' % path]]
28+
29+ def run_test(self):
30+ # generate 5 blocks
31+ blocks = self.nodes[0].generate(5)
32+ sleep(1)
runCommand()
is aync. Any idea to avoid the sleep without pooling or blocking reads?
0@@ -0,0 +1,42 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2017 The Bitcoin Core developers
3+# Distributed under the MIT software license, see the accompanying
4+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+"""Test blocknotify.
6+
7+Verify that a bitcoind node calls the given command for each block
8+"""
9+from os import close, remove
import os
. os.close
and os.remove
is used below :-)
We have a winner:
0$ git grep -E "(import os|from os)" -- "*.py" | cut -f2 -d: | sort | uniq -c
1 25 import os
2 2 import os.path
7+Verify that a bitcoind node calls the given command for each block
8+"""
9+from os import close, remove
10+from tempfile import mkstemp
11+from test_framework.test_framework import BitcoinTestFramework
12+from test_framework.util import assert_equal, assert_raises_jsonrpc
assert_raises_jsonrpc
. Not used :-)
16+
17+ def __init__(self):
18+ super().__init__()
19+
20+ # get a temporary file path to store the notified blocks
21+ (fd, path) = mkstemp(text = True)
fd, path = mkstemp(text=True)
will do :-)
19+
20+ def __init__(self):
21+ super().__init__()
22+
23+ # get a temporary file path to store the notified blocks
24+ fd, path = tempfile.mkstemp(text = True)
Please use self.options.tmpdir
, so that files are contained in a single tree for each test.
Also nit: Please no whitespace for named args. (text=True
)
self.options
is not defined at this point.
34+ def run_test(self):
35+ # generate 3 blocks
36+ blocks = self.nodes[0].generate(3)
37+
38+ # wait while the command runs asynchronously
39+ time.sleep(3)
Please no fixed sleeps. This might be too much time on a fast system or too less time on a slow io system.
You could wrap it into a while loop with timeout on failure and break on success. Alternatively there should be some helper function wait_until
if I am not mistaken.
Concept ACK, but needs travis fixed:
0WARNING! The following scripts are not being run: ['blocknotify.py']. Check the test lists in test_runner.py.
39+ time.sleep(3)
40+
41+ # file content should equal the generated blocks hashes
42+ with open(self.path, 'r') as f:
43+ assert_equal(''.join(blocks), f.read())
44+
nit: whitespace.
Consider using a linter to catch nits.
22+
23+ # get a temporary file path to store the notified blocks
24+ fd, path = tempfile.mkstemp(text = True)
25+ os.close(fd)
26+
27+ self.path = path
self.path
above rather than assign to a local variable path
A couple of nits to add to @MarcoFalke’s
You’ll need to add blocknotify.py
to BASE_SCRIPTS
in test_runner.py
You could try to overwrite setup_network:
0def setup_network():
1 self.extra_args = [bla]
2 super().setup_network()
On Mon, Jul 31, 2017 at 10:06 PM, João Barbosa notifications@github.com wrote:
@MarcoFalke https://github.com/marcofalke done. However can’t get tmpdir in the constructor. Any other suggestion?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/10941#issuecomment-319180756, or mute the thread https://github.com/notifications/unsubscribe-auth/AGGmv0EceWaRTPKc4AFnANOaqKf4O_Jqks5sTjOogaJpZM4OlIuV .
31+
32+ def run_test(self):
33+ block_count = 10
34+ blocks = self.nodes[0].generate(block_count)
35+
36+ # wait for expected data length
A few comments on this loop:
time.sleep(0.1)
should be fine.33+ block_count = 10
34+ blocks = self.nodes[0].generate(block_count)
35+
36+ # wait at most 10 seconds for expected file size before reading the content
37+ attempts = 0
38+ while os.stat(self.path).st_size < (block_count * 65) and attempts < 100:
Maybe wait until the file is created? I’d really suggest using some sort of wrapper, like the wait_until
from mininode
.
0wait_until(lambda: os.path.isfile(self.path), timeout=1)
1wait_until(lambda: os.stat(self.path).st_size >= (block_count * 65), timeout=10)
Just to make sure, that this is causing failures on my machine:
0 File "./test/functional/blocknotify.py", line 38, in run_test
1 while os.stat(self.path).st_size < (block_count * 65) and attempts < 100:
2FileNotFoundError: [Errno 2] No such file or directory: '/tmp/bitcoin_test_runner_20170815_171120/blocknotify_621/blocks.txt'
ping @promag: Are you still working on this?
I’m not seeing any errors when I run the test, but this is probably caused by a race between the start of this loop and the first notification. Could be fixed by adding a not os.path.exists(self.path)
check here, or adding with open(self.path, "w"): pass
to setup_network
.
Indeed. The code I posted a few comments above fixed it for me.
c.f.
0wait_until(lambda: os.path.isfile(self.path), timeout=1)
1wait_until(lambda: os.stat(self.path).st_size >= (block_count * 65), timeout=10)
24+
25+ def setup_network(self):
26+ self.path = os.path.join(self.options.tmpdir, 'blocks.txt')
27+
28+ # the command will append the notified block hash to file
29+ self.extra_args = [['-blocknotify=echo %%s >> %s' % self.path]]
shlex.quote(self.path)
tmpdir
is used elsewhere (should be secure) and blocks.txt
doesn’t need to be quoted. Is there a strong argument for it?
echo %%s >> %s
?
Tested ACK 2b03bc1fb844980ffc67c6eaa52b034e978a9826 as long as the race condition is fixed.
I agree with others the test setup should be merged with walletnotify test setup in #10966 so code is not duplicated, but I don’t think it matters if this is done in one or two prs.
wait_until
.
forknotify.py
needs to be changed.
32+ # wait at most 10 seconds for expected file size before reading the content
33+ wait_until(lambda: os.stat(self.path).st_size >= (block_count * 65), timeout=10)
34+
35+ # file content should equal the generated blocks hashes
36+ with open(self.path, 'r') as f:
37+ assert_equal(blocks, f.read().splitlines())
Hmm. I don’t think boost::thread guarantees that the thread run in the order they are constructed. Am I wrong?
The arrays should have the same content, but a different order.
Correct, in -walletnotify
(#10966) there’s:
0 # txids are sorted because there is no order guarantee in notifications
1 assert_equal(sorted(txids_rpc), sorted(txids_file))
So I’ll do the same here for correctness.
-rescan
to node[1]
(needs squash).
@MarcoFalke the block sort was already on the @jnewbery branch.
0@@ -0,0 +1,86 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2014-2016 The Bitcoin Core developers
3+# Distributed under the MIT software license, see the accompanying
4+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+"""Test the -alertnotify and -blocknotify options."""
6+import os
7+
8+from test_framework.test_framework import BitcoinTestFramework
9+from test_framework.util import assert_equal, wait_until, connect_nodes_bi
10+
11+class ForkNotifyTest(BitcoinTestFramework):
NotificationsTest
ACK https://github.com/bitcoin/bitcoin/pull/10941/commits/857b32b4b280f13997cf2fa471802ad6a27075fb
(or the squashed version of this)
I think makes sense to have individual commits.
Certainly if multiple authors are involved. And the different commits concern different aspects.
What we want to avoid with squashing is sequential commits like “fix the previous commit” which are internal to the author’s working and are only confusing when reading the history. But that’s not the case here.
utACK 857b32b