Add walletnotify functional test

promag commented at 9:39 pm on July 31, 2017: member

Similar to #10941, but for -walletnotify option.

promag force-pushed on Jul 31, 2017

practicalswift commented at 5:03 am on August 1, 2017: contributor

Concept ACK 👍

in test/functional/test_runner.py:152 in 647920c7cc outdated

148@@ -149,6 +149,7 @@
149     'invalidateblock.py',
150     'p2p-acceptblock.py',
151     'replace-by-fee.py',
152+    'walletnotify-py',

jnewbery commented at 5:54 pm on August 2, 2017:

should be 'walletnotify.py'

promag commented at 7:17 pm on August 2, 2017:

Yes.

jnewbery commented at 6:54 pm on August 2, 2017: member

This test currently fails for me:

0Traceback (most recent call last):
1  File "/home/ubuntu/bitcoin/test/functional/test_framework/test_framework.py", line 149, in main
2    self.run_test()
3  File "./walletnotify.py", line 61, in run_test
4    assert_equal(list(map(lambda t: t['txid'], self.nodes[0].listtransactions("*", block_count))), f.read().splitlines())
5  File "/home/ubuntu/bitcoin/test/functional/test_framework/util.py", line 36, in assert_equal
6    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
7AssertionError: not(['f9f2bb01dc512ad6ef8dff8de16995862891be39885f1b922426d83784fc17f3', 'a655a9f15488093e752a870c60578be1ec41e2d013bad183d865c221318bb26f', '07d8beee3fb2e18cd6458ef6d9263bf8a0f58fe81cd799eb4e6319777858ccf4', 'f875332c232f29ab931ff7964ab3ef062584255ce8c446c759091a2073ec4e08', 'ba7f2b8b19428a53f3e3f294bab54617d33232bdfdcfd59cd99a8ba15280575e', '6048b098928225e5a10d778602885009621416f1ce19509e5d39a7ae1bca8257', 'ab5972edc4f7dfe0d0cf8b4b38900e6d97e09bcdcfc43ae15b23ede4071deb32', '906720b1b720e6a4b9893ca2ff1dc11c42fe17ea82f039dfbe4119f616932e8e', 'c1226a7cca4cec6f4d6408a2e1f025267fa10ffd60b7d909a12faacf65fcfa5c', '1caf4ffcd307189f8189d7b76aea65b41eb25680e7207191912c83d54237b71f'] == ['f9f2bb01dc512ad6ef8dff8de16995862891be39885f1b922426d83784fc17f3', 'a655a9f15488093e752a870c60578be1ec41e2d013bad183d865c221318bb26f', '07d8beee3fb2e18cd6458ef6d9263bf8a0f58fe81cd799eb4e6319777858ccf4', 'ba7f2b8b19428a53f3e3f294bab54617d33232bdfdcfd59cd99a8ba15280575e', '6048b098928225e5a10d778602885009621416f1ce19509e5d39a7ae1bca8257', '1caf4ffcd307189f8189d7b76aea65b41eb25680e7207191912c83d54237b71f', 'ab5972edc4f7dfe0d0cf8b4b38900e6d97e09bcdcfc43ae15b23ede4071deb32', '906720b1b720e6a4b9893ca2ff1dc11c42fe17ea82f039dfbe4119f616932e8e', 'c1226a7cca4cec6f4d6408a2e1f025267fa10ffd60b7d909a12faacf65fcfa5c', 'f875332c232f29ab931ff7964ab3ef062584255ce8c446c759091a2073ec4e08'])

ordering is different between the lists.

promag commented at 7:17 pm on August 2, 2017: member

Makes sense since there are no order guarantees of notification commands. Should that be fixed or for now sort both arrays?

jnewbery commented at 7:35 pm on August 2, 2017: member

there are no order guarantees of notification commands. Should that be fixed or for now sort both arrays?

I think it’s fine not to change to sort the arrays in this test (as long as you add a comment to say that notifications are not guaranteed to be in order).

Feel free to open a separate pull request if you think notifications should be in order.

promag force-pushed on Aug 2, 2017

promag commented at 9:26 pm on August 2, 2017: member

@jnewbery if the out-of-order is not handled in the test, this will cause unrelated travis failures. If at the moment the order is not guaranteed, then the test must handle that IMO. In the future, if the order is guaranteed, then the test is adjusted.

promag commented at 9:27 pm on August 2, 2017: member

BTW, the same can happen with -blocknotify.

promag commented at 9:42 pm on August 2, 2017: member

@jnewbery pushed a new commit with a tiny refactor where I also address your comments from #10941.

jnewbery commented at 9:42 pm on August 2, 2017: member

I’ve been thinking: Does it make sense to combine this and blocknotify with the existing forknotify.py test?

jonasschnelli added the label Tests on Aug 3, 2017

jonasschnelli commented at 9:43 am on August 3, 2017: contributor

utACK b982d6db88910aa90ed3a78ef15401326e387f9b (after squash). I agree with @jnewbery about combining this (and rename) with forknotify.py (maybe rename notifyexec.py or notify.py). Could also be done in a later PR.

promag commented at 9:52 am on August 3, 2017: member

I tend to prefer isolated and simple tests. It’s easier to understand and simpler to maintain.

test: Add walletnotify functional test 7331ea404e

promag force-pushed on Aug 3, 2017

MarcoFalke renamed this:
~~Add walletnotify functional test~~
qa: Add walletnotify functional test
on Sep 13, 2017

in test/functional/walletnotify.py:20 in 7331ea404e

15+from test_framework.util import assert_equal
16+
17+class WalletNotifyTest(BitcoinTestFramework):
18+
19+    def __init__(self):
20+        super().__init__()

MarcoFalke commented at 6:56 pm on October 2, 2017:

Needs rebase due to silent merge conflict, I guess

in test/functional/walletnotify.py:50 in 7331ea404e

45+    def test_file_content(self):
46+        txids_rpc = list(map(lambda t: t['txid'], self.nodes[0].listtransactions("*", self.block_count)))
47+
48+        # wait at most 10 seconds for expected file size before reading the content
49+        attempts = 0
50+        while os.stat(self.path).st_size < (self.block_count * 65) and attempts < 100:

MarcoFalke commented at 6:57 pm on October 2, 2017:

can use wait_until, no?

promag commented at 7:02 pm on October 2, 2017: member

I’ll refresh this PR.

promag renamed this:
~~qa: Add walletnotify functional test~~
Add walletnotify functional test
on Oct 4, 2017

promag commented at 0:33 am on October 10, 2017: member

Merged in #10941 as per @jnewbery suggestion.

promag closed this on Oct 10, 2017

promag deleted the branch on Oct 10, 2017

DrahtBot locked this on Sep 8, 2021

Add walletnotify functional test #10966