Add walletnotify functional test #10966

pull promag wants to merge 1 commits into bitcoin:master from promag:2017-07-walletnotify-functional-test changing 2 files +63 −0

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:

Traceback (most recent call last):
  File "/home/ubuntu/bitcoin/test/functional/test_framework/test_framework.py", line 149, in main
    self.run_test()
  File "./walletnotify.py", line 61, in run_test
    assert_equal(list(map(lambda t: t['txid'], self.nodes[0].listtransactions("*", block_count))), f.read().splitlines())
  File "/home/ubuntu/bitcoin/test/functional/test_framework/util.py", line 36, in assert_equal
    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: 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 12: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

Contributors

Labels

Linked (view graph)

#10941 Add blocknotify and walletnotify functional tests