This patch adds the missing functional tests for -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.
Add blocknotify and walletnotify functional tests #10941
pull promag wants to merge 3 commits into bitcoin:master from promag:2017-07-blocknotify-functional-test changing 3 files +87 −60-
promag commented at 11:10 AM on July 27, 2017: member
-
in test/functional/blocknotify.py:32 in cebf6e05da outdated
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)
promag commented at 11:13 AM on July 27, 2017:Without sleep sometimes the file doesn't have all blocks because
runCommand()is aync. Any idea to avoid the sleep without pooling or blocking reads?laanwj added the label Tests on Jul 27, 2017in test/functional/blocknotify.py:9 in cebf6e05da outdated
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
practicalswift commented at 11:57 AM on July 27, 2017:Replace with
import os.os.closeandos.removeis used below :-)
promag commented at 12:00 PM on July 27, 2017:Lol yes. What do you guys prefer? Import module or symbols?
practicalswift commented at 12:10 PM on July 27, 2017:We have a winner:
$ git grep -E "(import os|from os)" -- "*.py" | cut -f2 -d: | sort | uniq -c 25 import os 2 import os.pathin test/functional/blocknotify.py:12 in cebf6e05da outdated
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
practicalswift commented at 11:57 AM on July 27, 2017:Remove
assert_raises_jsonrpc. Not used :-)in test/functional/blocknotify.py:21 in cebf6e05da outdated
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)
practicalswift commented at 11:58 AM on July 27, 2017:Nit:
fd, path = mkstemp(text=True)will do :-)practicalswift commented at 11:59 AM on July 27, 2017: contributorConcept ACK :-)
promag force-pushed on Jul 27, 2017promag commented at 9:35 PM on July 27, 2017: member@practicalswift thanks for the review. Added a few comments and fixed the nits.
promag force-pushed on Jul 27, 2017in test/functional/blocknotify.py:24 in 4e9b9b22ac outdated
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)
MarcoFalke commented at 9:13 AM on July 28, 2017: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)
promag commented at 10:13 AM on July 28, 2017:self.optionsis not defined at this point.in test/functional/blocknotify.py:39 in 4e9b9b22ac outdated
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)
MarcoFalke commented at 9:16 AM on July 28, 2017: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_untilif I am not mistaken.MarcoFalke commented at 9:17 AM on July 28, 2017: memberConcept ACK, but needs travis fixed:
WARNING! The following scripts are not being run: ['blocknotify.py']. Check the test lists in test_runner.py.in test/functional/blocknotify.py:44 in 4e9b9b22ac outdated
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 | +
jnewbery commented at 9:26 AM on July 28, 2017:nit: whitespace.
Consider using a linter to catch nits.
in test/functional/blocknotify.py:27 in 4e9b9b22ac outdated
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
jnewbery commented at 9:29 AM on July 28, 2017:can assign
self.pathabove rather than assign to a local variablepathjnewbery commented at 9:33 AM on July 28, 2017: memberA couple of nits to add to @MarcoFalke's
You'll need to add
blocknotify.pytoBASE_SCRIPTSin test_runner.pypromag force-pushed on Jul 28, 2017promag force-pushed on Jul 28, 2017promag force-pushed on Jul 31, 2017promag commented at 8:05 PM on July 31, 2017: member@MarcoFalke done. However can't get tmpdir in the constructor. Any other suggestion?
MarcoFalke commented at 8:15 PM on July 31, 2017: memberYou could try to overwrite setup_network:
def setup_network(): self.extra_args = [bla] 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 .
promag force-pushed on Jul 31, 2017promag force-pushed on Jul 31, 2017promag commented at 8:54 PM on July 31, 2017: member@MarcoFalke ty, all comments addressed.
promag force-pushed on Jul 31, 2017promag force-pushed on Jul 31, 2017practicalswift commented at 4:51 AM on August 1, 2017: contributorutACK d2c730c77f9b4011040e30f2bc2a4c8c6deb13f2
in test/functional/blocknotify.py:36 in d2c730c77f outdated
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
jnewbery commented at 5:37 PM on August 2, 2017:A few comments on this loop:
- I'd make the sleep longer (say 0.1 seconds). I'm also not sure you need the back-off. Just a
time.sleep(0.1)should be fine. - The total time allowed for the file to be written is 0.45 seconds. That needs to be longer (up to 10 seconds) since Travis can block i/o for many seconds at a time. See #10072.
- Please make the comment clear about the total time allowed for the loop.
jnewbery commented at 5:38 PM on August 2, 2017: membertested ACK with one change needed to the timing of the while loop
promag force-pushed on Aug 12, 2017promag force-pushed on Aug 12, 2017MarcoFalke commented at 11:29 AM on August 12, 2017: memberNeeds rebase
promag force-pushed on Aug 12, 2017promag commented at 1:56 PM on August 12, 2017: member@MarcoFalke done.
in test/functional/blocknotify.py:38 in 2b03bc1fb8 outdated
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:
MarcoFalke commented at 2:25 PM on August 12, 2017:Maybe wait until the file is created? I'd really suggest using some sort of wrapper, like the
wait_untilfrommininode.wait_until(lambda: os.path.isfile(self.path), timeout=1) wait_until(lambda: os.stat(self.path).st_size >= (block_count * 65), timeout=10)
MarcoFalke commented at 3:13 PM on August 15, 2017:Just to make sure, that this is causing failures on my machine:
File "./test/functional/blocknotify.py", line 38, in run_test while os.stat(self.path).st_size < (block_count * 65) and attempts < 100: FileNotFoundError: [Errno 2] No such file or directory: '/tmp/bitcoin_test_runner_20170815_171120/blocknotify_621/blocks.txt'
MarcoFalke commented at 6:59 PM on October 2, 2017:ping @promag: Are you still working on this?
ryanofsky commented at 1:27 PM on October 3, 2017: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 addingwith open(self.path, "w"): passtosetup_network.
MarcoFalke commented at 3:38 PM on October 3, 2017:Indeed. The code I posted a few comments above fixed it for me.
c.f.
wait_until(lambda: os.path.isfile(self.path), timeout=1) wait_until(lambda: os.stat(self.path).st_size >= (block_count * 65), timeout=10)in test/functional/blocknotify.py:24 in 2b03bc1fb8 outdated
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]]
ryanofsky commented at 1:21 PM on October 3, 2017:Maybe escape path with
shlex.quote(self.path)
promag commented at 2:12 PM on October 4, 2017:tmpdiris used elsewhere (should be secure) andblocks.txtdoesn't need to be quoted. Is there a strong argument for it?
promag commented at 2:16 PM on October 4, 2017:Of you mean to quote the command
echo %%s >> %s?
ryanofsky commented at 2:50 PM on October 4, 2017:Quoting the entire command would be incorrect because it is passed literally to system(3). Quoting the path is correct because it is being passed as a shell token. Anyway just suggested this in case you wanted the code to be more correct. There are no practical implications (security or otherwise) either way.
promag commented at 2:53 PM on October 4, 2017:Right, otherwise it wouldn't work 😃 I'm going for simplicity if you don't mind.
ryanofsky commented at 1:36 PM on October 3, 2017: memberTested 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.
promag commented at 6:59 AM on October 4, 2017: memberAs suggested I'll merge the test and use
wait_until.promag force-pushed on Oct 4, 2017promag force-pushed on Oct 4, 2017promag commented at 2:14 PM on October 4, 2017: memberRebased and updated as per suggestions. Still in a separate test file because IMO that should go to a different PR since
forknotify.pyneeds to be changed.jnewbery commented at 3:33 PM on October 4, 2017: memberPlease see https://github.com/jnewbery/bitcoin/tree/fork_notify. This adds blocknotify and walletnotify to the existint fork_notify.py test script.
in test/functional/blocknotify.py:37 in e7408a4447 outdated
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())
MarcoFalke commented at 7:41 PM on October 9, 2017: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.
promag commented at 12:20 AM on October 10, 2017:Correct, in
-walletnotify(#10966) there's:# txids are sorted because there is no order guarantee in notifications assert_equal(sorted(txids_rpc), sorted(txids_file))So I'll do the same here for correctness.
MarcoFalke commented at 7:41 PM on October 9, 2017: memberutACK e7408a444 but needs clarification
promag renamed this:Add blocknotify functional test
Add blocknotify and walletnotify functional tests
on Oct 10, 2017promag force-pushed on Oct 10, 2017promag commented at 12:37 AM on October 10, 2017: member@jnewbery Picked your branch and added a commit that adds
-rescantonode[1](needs squash). @MarcoFalke the block sort was already on the @jnewbery branch.jnewbery commented at 4:51 PM on October 10, 2017: memberTested ACK 6d9c1593497e9f295b5f335c7c7a0df7966d7590. As you've pointed out, should be squashed prior to merge.
promag force-pushed on Oct 10, 2017in test/functional/notifications.py:5 in 22a3487e22 outdated
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."""
mess110 commented at 10:42 PM on October 10, 2017:You are also testing -walletnotify, could you please update the comment?
in test/functional/notifications.py:11 in 22a3487e22 outdated
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):
mess110 commented at 10:42 PM on October 10, 2017:Is the class name still correct? I think it should be something like
NotificationsTest[tests] Tidy up forknotify.py 9c72a464f8promag force-pushed on Oct 10, 2017[tests] Add -blocknotify functional test df18d29a02[tests] Add -walletnotify functional test 857b32b4b2promag force-pushed on Oct 10, 2017mess110 commented at 10:49 PM on October 10, 2017: contributorACK https://github.com/bitcoin/bitcoin/pull/10941/commits/857b32b4b280f13997cf2fa471802ad6a27075fb
(or the squashed version of this)
promag commented at 10:52 PM on October 10, 2017: memberI think makes sense to have individual commits.
laanwj commented at 9:24 AM on October 11, 2017: memberI 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
laanwj merged this on Oct 11, 2017laanwj closed this on Oct 11, 2017practicalswift referenced this in commit 364da2c529 on Oct 11, 2017PastaPastaPasta referenced this in commit 2a7b47220b on Dec 22, 2019PastaPastaPasta referenced this in commit 6d164bf997 on Jan 2, 2020PastaPastaPasta referenced this in commit a7ced12270 on Jan 4, 2020PastaPastaPasta referenced this in commit e618e3f088 on Jan 12, 2020PastaPastaPasta referenced this in commit 137e0a6509 on Jan 12, 2020PastaPastaPasta referenced this in commit 7ba71fd26b on Jan 12, 2020PastaPastaPasta referenced this in commit efebec8a9f on Jan 12, 2020PastaPastaPasta referenced this in commit 177696874d on Jan 12, 2020PastaPastaPasta referenced this in commit 31bdc10c51 on Jan 12, 2020PastaPastaPasta referenced this in commit 7f2ebba617 on Jan 12, 2020ckti referenced this in commit a6a5bfc7e0 on Mar 28, 2021MarcoFalke locked this on Sep 8, 2021Labels
github-metadata-mirror
This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-21 18:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me