Skip to content

test: move accounting_tests and rpc_wallet_tests to wallet/test #7905

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
Merged
merged 4 commits into from
Apr 19, 2016
Merged

test: move accounting_tests and rpc_wallet_tests to wallet/test #7905

merged 4 commits into from
Apr 19, 2016

Conversation

laanwj
Copy link
Member

@laanwj laanwj commented Apr 18, 2016

Move the two other wallet tests to where they belong.

@jonasschnelli I'm surprised you didn't stumble on this one yet

@laanwj
test: move accounting_tests and rpc_wallet_tests to wallet/test
de39c95 
Move the two other wallet tests to where they belong.
@jonasschnelli
Copy link
Contributor

Yes! This should have been done long ago. Thanks for fixing.
utACK de39c95

@laanwj
Copy link
Member Author

laanwj commented Apr 18, 2016

With this, I guess we also want a wallet-specific test fixture, to get rid of the ENABLE_WALLETs in test_bitcoin.cpp?

@laanwj
test: Create test fixture for wallet
f4eae2d 
Removes all the `#ifdef ENABLE_WALLET` from `test_bitcoin` by
making the wallet tests use their own fixture.
@laanwj laanwj force-pushed the 2016_04_accounting_tests_to_wallet branch from 8a1c078 to f4eae2d Compare April 18, 2016 12:57
@laanwj
Copy link
Member Author

laanwj commented Apr 18, 2016

Done. Latest commit removes all wallet-related setup from test_bitcoin.cpp. Wallet tests set up and tear down their own environment.

These references to the wallet remain in src/test, this probably signifies some code movement is still necessary:

multisig_tests.cpp:#include "wallet/wallet_ismine.h"
script_P2SH_tests.cpp:#include "wallet/wallet_ismine.h"

@sipa
Copy link
Member

sipa commented Apr 18, 2016 via email

maflcko
maflcko reviewed Apr 18, 2016
View reviewed changes
src/wallet/test/wallet_test_fixture.cpp
bitdb.MakeMock();

bool fFirstRun;
pwalletMain = new CWallet("wallet.dat");
Copy link
Member

Choose a reason for hiding this comment

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

Could make sense to use "wallet_test.dat" here, to indicate that the file name is not hardcoded?

Copy link
Member Author

@laanwj laanwj Apr 18, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Sounds good to me in principle (although I just moved this code here, I didn't touch it)

Edit: done in a separate commit

@laanwj
Copy link
Member Author

laanwj commented Apr 18, 2016

@sipa What about keeping it separate and just moving, e.g. script/ismine.cpp and script/ismine.h?

laanwj added 2 commits April 18, 2016 15:14
@laanwj
wallet_ismine.h → script/ismine.h
a25a4f5 
Removes conditional dependency of `src/test` on wallet.

Makes multisig and P2SH tests complete without wallet built-in.
@laanwj
test: Rename wallet.dat to wallet_test.dat
b30fb42 
Indicate that the file name is not hardcoded, and a little bit of safety
so that it never nukes the main wallet.

Suggestion by Marco Falke.
@laanwj laanwj force-pushed the 2016_04_accounting_tests_to_wallet branch from f2f572c to b30fb42 Compare April 18, 2016 13:17
@laanwj
Copy link
Member Author

laanwj commented Apr 18, 2016

moving, e.g. script/ismine.cpp and script/ismine.h?

Done. All conditional wallet stuff is gone now from src/test.

@maflcko
Copy link
Member

maflcko commented Apr 18, 2016

Confirmed move only besides b30fb42

utACK b30fb42

@laanwj laanwj merged commit b30fb42 into bitcoin:master Apr 19, 2016
laanwj added a commit that referenced this pull request Apr 19, 2016
@laanwj
Merge #7905: test: move accounting_tests and rpc_wallet_tests to wall…
187186b 
…et/test

b30fb42 test: Rename wallet.dat to wallet_test.dat (Wladimir J. van der Laan)
a25a4f5 wallet_ismine.h → script/ismine.h (Wladimir J. van der Laan)
f4eae2d test: Create test fixture for wallet (Wladimir J. van der Laan)
de39c95 test: move accounting_tests and rpc_wallet_tests to wallet/test (Wladimir J. van der Laan)
@laanwj laanwj mentioned this pull request Apr 28, 2016
Closed
14 tasks
@sickpig sickpig mentioned this pull request Feb 22, 2017
Merged
codablock pushed a commit to codablock/dash that referenced this pull request Dec 20, 2017
@laanwj @codablock
Merge bitcoin#7905: test: move accounting_tests and rpc_wallet_tests …
8678f2b 
…to wallet/test

b30fb42 test: Rename wallet.dat to wallet_test.dat (Wladimir J. van der Laan)
a25a4f5 wallet_ismine.h → script/ismine.h (Wladimir J. van der Laan)
f4eae2d test: Create test fixture for wallet (Wladimir J. van der Laan)
de39c95 test: move accounting_tests and rpc_wallet_tests to wallet/test (Wladimir J. van der Laan)
@dagurval dagurval mentioned this pull request Jan 24, 2018
Merged
@str4d str4d mentioned this pull request Dec 18, 2019
Merged
zkbot added a commit to zcash/zcash that referenced this pull request Dec 18, 2019
@zkbot
Auto merge of #4270 - str4d:2074-wallet-2, r=str4d
fa341bc 
Bitcoin wallet PRs 2

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#7576
- bitcoin/bitcoin#7577
- bitcoin/bitcoin#7608
- bitcoin/bitcoin#7691
- bitcoin/bitcoin#7905
@random-zebra random-zebra mentioned this pull request Apr 1, 2020
Merged
@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
No reviews
Assignees
No one assigned
Labels
Build system Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@laanwj @jonasschnelli @sipa @maflcko