test: Add resendwallettransactions functional tests

promag commented at 3:19 pm on August 7, 2017: member

Adds functional tests to cover the behaviour introduced in #10995.

laanwj added the label Tests on Aug 7, 2017

promag force-pushed on Aug 7, 2017

TheBlueMatt commented at 3:39 pm on August 7, 2017: member

Thanks! Can you add a test to restart the node with walletbroadcast and check that it works? Maybe move the walletbroadcast tests from wallet.py?

promag commented at 3:50 pm on August 7, 2017: member

Can you add a test to restart the node with walletbroadcast and check that it works?

You mean restart with explicit arg (=true)?

Maybe move the walletbroadcast tests from wallet.py?

Maybe only the one calling resendwallettransactions?

TheBlueMatt commented at 6:47 pm on August 7, 2017: member

You mean restart with explicit arg (=true)?

Can do that too, just always a good idea to test failure and success conditions together.

Maybe only the one calling resendwallettransactions?

I just figured if you move all the walletbroadcasting stuff to one place (wallet.py is already big enough) but don’t really care.

in test/functional/resendwallettransactions.py:8 in 0dabe79833 outdated

0@@ -0,0 +1,31 @@
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 resendwallettransactions RPC."""
6+
7+from test_framework.test_framework import BitcoinTestFramework
8+from test_framework.util import *

jnewbery commented at 7:31 pm on August 7, 2017:

nit: prefer explicit import if you’re only using one function from test_framework.util

in test/functional/resendwallettransactions.py:21 in 0dabe79833 outdated

16+
17+    def run_test(self):
18+        # Should raise a RPC error if walletbroadcast is disabled.
19+        assert_raises_jsonrpc(-32600, "Error: Wallet transaction broadcasting is disabled with -walletbroadcast", self.nodes[0].resendwallettransactions)
20+
21+        # Should return an empty array if there aren't mempool transactions.

jnewbery commented at 7:44 pm on August 7, 2017:

Comment is incorrect. This isn’t about transactions in the mempool, it’s about transactions in the wallet.

in test/functional/resendwallettransactions.py:26 in 0dabe79833 outdated

21+        # Should return an empty array if there aren't mempool transactions.
22+        self.stop_node(0)
23+        self.nodes[0] = self.start_node(0, self.options.tmpdir)
24+        assert_equal(self.nodes[0].resendwallettransactions(), [])
25+
26+        # Should return an array with the mempool transaction.

jnewbery commented at 7:44 pm on August 7, 2017:

As above, this isn’t about mempool

jnewbery commented at 7:47 pm on August 7, 2017: member

Each test script incurs about 5 seconds of overhead (node startup and shutdown), so it’s much better to group tests for related functionality together into a single script. I think it makes sense to include this test in wallet.py.

promag force-pushed on Aug 7, 2017

promag commented at 9:24 pm on August 7, 2017: member

Thanks @jnewbery, comments addressed, and rebased with #11002.

Regarding where to place the tests, IMO performance issues shouldn’t lead to long-and-hard-to-follow-and-also-tedious-to-change tests. It also makes parallel tests hard to implement. Beside that, in this particular case, at least one restart must occur.

jnewbery commented at 9:59 pm on August 7, 2017: member

@promag and I chatted about this. I still prefer combining this with wallet.py to avoid additional test setup/teardown overhead (if it can be done in a way that doesn’t make the test less clear or maintainable).

I’m happy to defer to consensus opinion if other people disagree.

laanwj referenced this in commit 4268426b45 on Aug 8, 2017

sdaftuar commented at 3:09 pm on August 11, 2017: member

ACK f9853283623df7ab3972611005b9dd089b6985ca @jnewbery I don’t like long test-suite run times either, but in this case merging a test with the already hard-to-read wallet.py seems insufficiently compelling, especially since this is already done and working and is an easy test to read/understand/maintain.

jnewbery commented at 3:38 pm on August 11, 2017: member

@sdaftuar - sure, this is very much a case-by-case judgement call. Perhaps Matt’s suggestion of having a separate test for all the wallet broadcast functionality is the best way to organise these tests.

In any case, I certainly won’t NACK any new tests based on my personal taste, and we can always refactor in later PRs if it makes sense to group things differently.

test: Add resendwallettransactions functional tests bdf607e438

promag force-pushed on Aug 12, 2017

promag commented at 3:44 am on August 12, 2017: member

Rebased since #11002 is merged.

MarcoFalke merged this on Aug 12, 2017

MarcoFalke closed this on Aug 12, 2017

MarcoFalke referenced this in commit ac016e17d2 on Aug 12, 2017

PastaPastaPasta referenced this in commit 33807c8cee on Aug 2, 2019

UdjinM6 referenced this in commit da63a2deac on Aug 4, 2019

PastaPastaPasta referenced this in commit f1f9e5dfb5 on Aug 5, 2019

PastaPastaPasta referenced this in commit bb45e2df66 on Aug 6, 2019

PastaPastaPasta referenced this in commit 44dc84e69a on Aug 6, 2019

PastaPastaPasta referenced this in commit b0aef55115 on Aug 6, 2019

barrystyle referenced this in commit 18103aa09c on Jan 22, 2020

barrystyle referenced this in commit 0b532c71fb on Jan 22, 2020

DrahtBot locked this on Sep 8, 2021

test: Add resendwallettransactions functional tests #11000