test: Implement unit tests for CWallet::CreateTransaction #3667
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Ready for review! |
|
You should probably move #3668 into this PR, cause unit tests need to be passing for us to merge this |
|
I'd prefer rebasing this on top of develop after 3668 is merged. |
PastaPastaPasta
requested changes
Sep 3, 2020
src/wallet/test/wallet_tests.cpp
Outdated
| return false; | ||
| } | ||
| // Verify the number of requested outputs does match the resulting outputs | ||
| CheckEqual(vecEntries.size(), wtx.tx->vout.size() - (nChangePos != -1 ? 1 : 0)); |
There was a problem hiding this comment.
Why not just use BOOST_CHECK_EQUAL here?
There was a problem hiding this comment.
What about 29b9e0f? Saves one line :D
src/wallet/test/wallet_tests.cpp
Outdated
Comment on lines
857
to
862
| BOOST_CHECK(CreateTransaction({{10, false}}, false)); | ||
| BOOST_CHECK(CreateTransaction({{10, true}}, false)); | ||
| BOOST_CHECK(CreateTransaction({{100, false}}, false)); | ||
| BOOST_CHECK(CreateTransaction({{100, true}}, false)); | ||
| BOOST_CHECK(CreateTransaction({{10000, true}, {100, true}}, false)); | ||
| BOOST_CHECK(CreateTransaction({{10000, false}, {100, false}}, false)); |
There was a problem hiding this comment.
probably should just dust - 1, dust, and dust+1
There was a problem hiding this comment.
Hm i just dropped those initial tests, c7e34f5. They are anyway covered in the rest of the test cases.
At this point its modified from an other test when running all unit tests which lets this test fail unexpectedly.
Test if the wallet creation fails properly with the correct error messages.
xdustinface
force-pushed
the
pr-test-createtransaction
branch
from
c7e34f5 to
2190f80
Compare
xdustinface
added a commit
to xdustinface/dash
that referenced
this pull request
Sep 12, 2020
* test: Implement unit tests for CWallet::CreateTransaction * test: Reset minRelayTxFee in CreateTransactionTest At this point its modified from an other test when running all unit tests which lets this test fail unexpectedly. * test: Lock cs_main/cs_wallet when required in CreateTransactionTest * test: Add new test in CreateTransactionTest Test if the wallet creation fails properly with the correct error messages. * test: Fixed expected test results for some CreateTransactionTest cases * test: Fail if CreateTransaction runs into "Exceed max tries" case * test: Adjust last return in CreateTransaction * test: Drop dust tests
xdustinface
added a commit
to xdustinface/dash
that referenced
this pull request
Sep 12, 2020
* test: Implement unit tests for CWallet::CreateTransaction * test: Reset minRelayTxFee in CreateTransactionTest At this point its modified from an other test when running all unit tests which lets this test fail unexpectedly. * test: Lock cs_main/cs_wallet when required in CreateTransactionTest * test: Add new test in CreateTransactionTest Test if the wallet creation fails properly with the correct error messages. * test: Fixed expected test results for some CreateTransactionTest cases * test: Fail if CreateTransaction runs into "Exceed max tries" case * test: Adjust last return in CreateTransaction * test: Drop dust tests
xdustinface
added a commit
to xdustinface/dash
that referenced
this pull request
Sep 12, 2020
* test: Implement unit tests for CWallet::CreateTransaction * test: Reset minRelayTxFee in CreateTransactionTest At this point its modified from an other test when running all unit tests which lets this test fail unexpectedly. * test: Lock cs_main/cs_wallet when required in CreateTransactionTest * test: Add new test in CreateTransactionTest Test if the wallet creation fails properly with the correct error messages. * test: Fixed expected test results for some CreateTransactionTest cases * test: Fail if CreateTransaction runs into "Exceed max tries" case * test: Adjust last return in CreateTransaction * test: Drop dust tests
xdustinface
added a commit
to xdustinface/dash
that referenced
this pull request
Sep 12, 2020
* test: Implement unit tests for CWallet::CreateTransaction * test: Reset minRelayTxFee in CreateTransactionTest At this point its modified from an other test when running all unit tests which lets this test fail unexpectedly. * test: Lock cs_main/cs_wallet when required in CreateTransactionTest * test: Add new test in CreateTransactionTest Test if the wallet creation fails properly with the correct error messages. * test: Fixed expected test results for some CreateTransactionTest cases * test: Fail if CreateTransaction runs into "Exceed max tries" case * test: Adjust last return in CreateTransaction * test: Drop dust tests
gades
pushed a commit
to cosanta/cosanta-core
that referenced
this pull request
Mar 11, 2022
* test: Implement unit tests for CWallet::CreateTransaction * test: Reset minRelayTxFee in CreateTransactionTest At this point its modified from an other test when running all unit tests which lets this test fail unexpectedly. * test: Lock cs_main/cs_wallet when required in CreateTransactionTest * test: Add new test in CreateTransactionTest Test if the wallet creation fails properly with the correct error messages. * test: Fixed expected test results for some CreateTransactionTest cases * test: Fail if CreateTransaction runs into "Exceed max tries" case * test: Adjust last return in CreateTransaction * test: Drop dust tests
5 tasks
PastaPastaPasta
pushed a commit
that referenced
this pull request
Feb 9, 2024
…hify help text, undashify code comments (#5852) ## Issue being fixed or feature implemented This pull request is a follow-up to [some](#5834 (comment)) [feedback](#5834 (comment)) received on [dash#5834](#5834) as the patterns highlighted were present in different parts of the codebase and hence not corrected within the PR itself but addressed separately. This is that separate PR 🙂 (with some additional cleanup of my own) ## What was done? * This pull request will remain a draft until [dash#5834](#5834) as it will introduce more changes that will need to be corrected in this PR. * Code introduced that is unique to Dash Core (CoinJoin, InstantSend, etc.) has been excluded from un-Dashification as the purpose of it is to reduce backport conflicts, which don't apply in those cases. * `CWallet::CreateTransaction` and the `CreateTransactionTest` fixture have been excluded as the former originates from [dash#3668](#3668) and the latter from [dash#3667](#3667) and are distinct enough to be unique to Dash Core. * There are certain Dashifications and SegWit-removals that prove frustrating as it would break compatibility with programs that rely on the naming of certain keys * `getrawmempool`, `getmempoolancestors`, `getmempooldescendants` and `getmempoolentry` return `vsize` which is currently an alias of `size`. I have been advised to retain `vsize` in lieu of potential future developments. (this was originally remedied in 219a1d0 but has since been dropped) * `getaddressmempool`, `getaddressutxos` and `getaddressdeltas` all return a value with the key `satoshis`. This is frustrating to rename to `duffs` for compatibility reasons. * `decodepsbt` returns (if applicable) `non_witness_utxo` which is frustrating to rename simply to `utxo` for the same reason. * `analyzepsbt` returns (if applicable) `estimated_vsize` which frustrating to rename to `estimated_size` for the same reason. ## How Has This Been Tested? ## Breaking Changes None ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
5 tasks
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I decided to add this because i worked on a fix for
CreateTransactionand while doing that i figured its very hard to say for sure by just manual test/reviewing if things are still good since this is quite complex method imo with a lot possible routes. I think this should catch all currently possible cases in there. If someone can think about any improvement or more test cases, let me know!Requires #3666 and #3668 to pass all the tests.