Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test: Add resendwallettransactions functional tests #11000

Merged
merged 1 commit into from
Aug 12, 2017
Merged

test: Add resendwallettransactions functional tests #11000

merged 1 commit into from
Aug 12, 2017

Conversation

promag
Copy link
Member

@promag promag commented Aug 7, 2017 •
edited
Loading

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

@laanwj laanwj added the Tests label Aug 7, 2017
@promag promag force-pushed the 201708-resendwallettransactions-test branch from ae384aa to 0dabe79 Compare August 7, 2017 15:32
@TheBlueMatt
Copy link
Contributor

TheBlueMatt commented Aug 7, 2017 •
edited
Loading

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
Copy link
Member Author

promag commented Aug 7, 2017

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
Copy link
Contributor

TheBlueMatt commented Aug 7, 2017 via email

jnewbery
jnewbery reviewed Aug 7, 2017
View reviewed changes
Copy link
Contributor

@jnewbery jnewbery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

test/functional/resendwallettransactions.py Outdated
"""Test resendwallettransactions RPC."""

from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import *
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

test/functional/resendwallettransactions.py Outdated
# Should raise a RPC error if walletbroadcast is disabled.
assert_raises_jsonrpc(-32600, "Error: Wallet transaction broadcasting is disabled with -walletbroadcast", self.nodes[0].resendwallettransactions)

# Should return an empty array if there aren't mempool transactions.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

test/functional/resendwallettransactions.py Outdated
self.nodes[0] = self.start_node(0, self.options.tmpdir)
assert_equal(self.nodes[0].resendwallettransactions(), [])

# Should return an array with the mempool transaction.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, this isn't about mempool

@jnewbery jnewbery mentioned this pull request Aug 7, 2017
Merged
@promag promag force-pushed the 201708-resendwallettransactions-test branch from 0dabe79 to f985328 Compare August 7, 2017 21:18
@promag
Copy link
Member Author

promag commented Aug 7, 2017

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
Copy link
Contributor

jnewbery commented Aug 7, 2017

@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.

@promag promag mentioned this pull request Aug 8, 2017
Merged
laanwj added a commit that referenced this pull request Aug 8, 2017
@laanwj
Merge #11002: [wallet] return correct error code from resendwallettra…
4268426 
…nsaction

055d95f [wallet] return correct error code from resendwallettransaction (John Newbery)

Pull request description:

  New code in #10995 uses `RPC_INVALID_REQUEST`. According to the comment in rpc/protocol.h:
  ```
  // RPC_INVALID_REQUEST is internally mapped to HTTP_BAD_REQUEST (400).
  // It should not be used for application-layer errors.
  ```
  Change the returned error code to `RPC_WALLET_ERROR`

  #11000 will need to be updated to test for the correct error code.

Tree-SHA512: 0201b3a2091adf17ad301825da5bd29f0ea7e284b5394cbef80483fc293a558acc849f74a0780bb8501acab324fc722e41ae049cffec7afb76884e26df4b809e
@sdaftuar
Copy link
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
Copy link
Contributor

@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.

@promag promag force-pushed the 201708-resendwallettransactions-test branch from f985328 to bdf607e Compare August 12, 2017 03:43
@promag
Copy link
Member Author

promag commented Aug 12, 2017

Rebased since #11002 is merged.

@maflcko maflcko merged commit bdf607e into bitcoin:master Aug 12, 2017
maflcko pushed a commit that referenced this pull request Aug 12, 2017
Merge #11000: test: Add resendwallettransactions functional tests
ac016e1 
bdf607e test: Add resendwallettransactions functional tests (João Barbosa)

Pull request description:

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

Tree-SHA512: 3be337cbe51edab2cc0eb9ecfa68d00cd3c3a00aef0672cd841706802726c9cd4138626a8f209c3d9fbd207ce332daa062f270e4c94c1c2398bfc475e36423f6
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Aug 2, 2019
@PastaPastaPasta
Merge bitcoin#11000: test: Add resendwallettransactions functional tests
33807c8 
bdf607e test: Add resendwallettransactions functional tests (João Barbosa)

Pull request description:

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

Tree-SHA512: 3be337cbe51edab2cc0eb9ecfa68d00cd3c3a00aef0672cd841706802726c9cd4138626a8f209c3d9fbd207ce332daa062f270e4c94c1c2398bfc475e36423f6
UdjinM6 pushed a commit to UdjinM6/dash that referenced this pull request Aug 4, 2019
@laanwj @UdjinM6
Merge bitcoin#11002: [wallet] return correct error code from resendwa…
da63a2d 
…llettransaction

055d95f [wallet] return correct error code from resendwallettransaction (John Newbery)

Pull request description:

  New code in bitcoin#10995 uses `RPC_INVALID_REQUEST`. According to the comment in rpc/protocol.h:
  ```
  // RPC_INVALID_REQUEST is internally mapped to HTTP_BAD_REQUEST (400).
  // It should not be used for application-layer errors.
  ```
  Change the returned error code to `RPC_WALLET_ERROR`

  bitcoin#11000 will need to be updated to test for the correct error code.

Tree-SHA512: 0201b3a2091adf17ad301825da5bd29f0ea7e284b5394cbef80483fc293a558acc849f74a0780bb8501acab324fc722e41ae049cffec7afb76884e26df4b809e
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Aug 5, 2019
@laanwj @PastaPastaPasta
Merge bitcoin#11002: [wallet] return correct error code from resendwa…
f1f9e5d 
…llettransaction

055d95f [wallet] return correct error code from resendwallettransaction (John Newbery)

Pull request description:

  New code in bitcoin#10995 uses `RPC_INVALID_REQUEST`. According to the comment in rpc/protocol.h:
  ```
  // RPC_INVALID_REQUEST is internally mapped to HTTP_BAD_REQUEST (400).
  // It should not be used for application-layer errors.
  ```
  Change the returned error code to `RPC_WALLET_ERROR`

  bitcoin#11000 will need to be updated to test for the correct error code.

Tree-SHA512: 0201b3a2091adf17ad301825da5bd29f0ea7e284b5394cbef80483fc293a558acc849f74a0780bb8501acab324fc722e41ae049cffec7afb76884e26df4b809e
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Aug 6, 2019
@PastaPastaPasta
Merge bitcoin#11000: test: Add resendwallettransactions functional tests
bb45e2d 
bdf607e test: Add resendwallettransactions functional tests (João Barbosa)

Pull request description:

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

Tree-SHA512: 3be337cbe51edab2cc0eb9ecfa68d00cd3c3a00aef0672cd841706802726c9cd4138626a8f209c3d9fbd207ce332daa062f270e4c94c1c2398bfc475e36423f6
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Aug 6, 2019
@laanwj @PastaPastaPasta
Merge bitcoin#11002: [wallet] return correct error code from resendwa…
44dc84e 
…llettransaction

055d95f [wallet] return correct error code from resendwallettransaction (John Newbery)

Pull request description:

  New code in bitcoin#10995 uses `RPC_INVALID_REQUEST`. According to the comment in rpc/protocol.h:
  ```
  // RPC_INVALID_REQUEST is internally mapped to HTTP_BAD_REQUEST (400).
  // It should not be used for application-layer errors.
  ```
  Change the returned error code to `RPC_WALLET_ERROR`

  bitcoin#11000 will need to be updated to test for the correct error code.

Tree-SHA512: 0201b3a2091adf17ad301825da5bd29f0ea7e284b5394cbef80483fc293a558acc849f74a0780bb8501acab324fc722e41ae049cffec7afb76884e26df4b809e
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Aug 6, 2019
@laanwj @PastaPastaPasta
Merge bitcoin#11002: [wallet] return correct error code from resendwa…
b0aef55 
…llettransaction

055d95f [wallet] return correct error code from resendwallettransaction (John Newbery)

Pull request description:

  New code in bitcoin#10995 uses `RPC_INVALID_REQUEST`. According to the comment in rpc/protocol.h:
  ```
  // RPC_INVALID_REQUEST is internally mapped to HTTP_BAD_REQUEST (400).
  // It should not be used for application-layer errors.
  ```
  Change the returned error code to `RPC_WALLET_ERROR`

  bitcoin#11000 will need to be updated to test for the correct error code.

Tree-SHA512: 0201b3a2091adf17ad301825da5bd29f0ea7e284b5394cbef80483fc293a558acc849f74a0780bb8501acab324fc722e41ae049cffec7afb76884e26df4b809e
barrystyle pushed a commit to PACGlobalOfficial/PAC that referenced this pull request Jan 22, 2020
@barrystyle
Merge bitcoin#11000: test: Add resendwallettransactions functional tests
18103aa 
bdf607e test: Add resendwallettransactions functional tests (João Barbosa)

Pull request description:

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

Tree-SHA512: 3be337cbe51edab2cc0eb9ecfa68d00cd3c3a00aef0672cd841706802726c9cd4138626a8f209c3d9fbd207ce332daa062f270e4c94c1c2398bfc475e36423f6
barrystyle pushed a commit to PACGlobalOfficial/PAC that referenced this pull request Jan 22, 2020
@laanwj @barrystyle
Merge bitcoin#11002: [wallet] return correct error code from resendwa…
0b532c7 
…llettransaction

055d95f [wallet] return correct error code from resendwallettransaction (John Newbery)

Pull request description:

  New code in bitcoin#10995 uses `RPC_INVALID_REQUEST`. According to the comment in rpc/protocol.h:
  ```
  // RPC_INVALID_REQUEST is internally mapped to HTTP_BAD_REQUEST (400).
  // It should not be used for application-layer errors.
  ```
  Change the returned error code to `RPC_WALLET_ERROR`

  bitcoin#11000 will need to be updated to test for the correct error code.

Tree-SHA512: 0201b3a2091adf17ad301825da5bd29f0ea7e284b5394cbef80483fc293a558acc849f74a0780bb8501acab324fc722e41ae049cffec7afb76884e26df4b809e
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jnewbery jnewbery jnewbery left review comments

Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@promag @TheBlueMatt @jnewbery @sdaftuar @laanwj @maflcko