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.

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

qa: Run all tests even if wallet is not compiled #14180

Merged
merged 2 commits into from
Sep 13, 2018

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Sep 9, 2018

Currently the test_runner would exit if the wallet was not compiled into the Bitcoin Core executable. However, a lot of the tests run without the wallet just fine and there is no need to globally require the wallet to run the tests.

@maflcko maflcko added the Tests label Sep 9, 2018
@maflcko
Copy link
Member Author

maflcko commented Sep 9, 2018

Travis run for reference: https://travis-ci.org/bitcoin/bitcoin/jobs/426386820#L2937

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 9, 2018

Note to reviewers: This pull request conflicts with the following ones:

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@lucash-dev
Copy link
Contributor

This PR includes a commit that's also present in #14179 (fae497d). What's the relationship between the two PRs?

@maflcko
Copy link
Member Author

maflcko commented Sep 10, 2018

This is based on #14179, since that is a requirement.

@Sjors
Copy link
Member

Sjors commented Sep 10, 2018

Concept ACK. It's useful to (occasionally) run the test suite without wallet (and other stuff) compiled.

Quite a few tests are skipped this way, but that can be improved later. That might require pre-generated transactions (only when --disable-wallet), or the ability for a node to generate transactions without having full wallet functionality.

How will this interact with #10102 (multi process), which has one binary with and one without a wallet? cc @ryanofsky. I imagine a common test configuration involves one wallet binary to generate transactions as well as one node binary.

I tested on macOS (only the --disable-wallet variant).

WARNING! There is already a bitcoind process running on this system. Tests may fail unexpectedly due to resource contention!

wallet_hd.py skipped

Can you add one WARNING! at the top for each disabled feature, so it's more clear what caused the skipped lines?

@maflcko
Copy link
Member Author

maflcko commented Sep 10, 2018

Quite a few tests are skipped this way, but that can be improved later.

Agree that this can be improved later. Skipping quite a few tests is better than skipping all tests.

if not wallet_enabled:
continue

n.importprivkey(n.get_deterministic_priv_key()[1])
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: could invert the conditional / drop the continue

@maflcko maflcko force-pushed the Mf1809-qaRunAllTheTests branch 2 times, most recently from cb49baa to fad40c9 Compare September 10, 2018 21:02
@maflcko maflcko force-pushed the Mf1809-qaRunAllTheTests branch from fad40c9 to fac9539 Compare September 10, 2018 21:53
@laanwj
Copy link
Member

laanwj commented Sep 11, 2018

Agree that this can be improved later. Skipping quite a few tests is better than skipping all tests.

👍

I do think this combination needs to be tested in Travis, though, or it's going to code-rot. At least I never run the tests without wallet.

@Sjors
Copy link
Member

Sjors commented Sep 11, 2018

@laanwj the x86_64 Linux, No wallet Travis host already runs this:
schermafbeelding 2018-09-11 om 15 47 39

@jnewbery
Copy link
Contributor

Concept ACK

@laanwj
Copy link
Member

laanwj commented Sep 12, 2018

utACK

@laanwj laanwj merged commit fac9539 into bitcoin:master Sep 13, 2018
laanwj added a commit that referenced this pull request Sep 13, 2018
fac9539 qa: Run all tests even if wallet is not compiled (MarcoFalke)
faa669c qa: Premine to deterministic address with -disablewallet (MarcoFalke)

Pull request description:

  Currently the test_runner would exit if the wallet was not compiled into the Bitcoin Core executable. However, a lot of the tests run without the wallet just fine and there is no need to globally require the wallet to run the tests.

Tree-SHA512: 63177260aa29126fd20f0be217a82b10b62288ab846f96f1cbcc3bd2c52702437703475d91eae3f8d821a3149fc62b725a4c5b2a7b3657b67ffcbc81532a03bb
@ryanofsky
Copy link
Contributor

Note: Some code review comments on this PR can be found in #14179 instead of here, since this PR was originally based on #14179, but I think got accidentally merged first.

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Oct 25, 2018
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Oct 25, 2018
toxeus pushed a commit to toxeus/bitcoin that referenced this pull request Nov 28, 2018
toxeus pushed a commit to toxeus/bitcoin that referenced this pull request Nov 28, 2018
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 20, 2021
…h -disablewallet

Signed-off-by: pasta <pasta@dashboost.org>
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 21, 2021
…h -disablewallet

Signed-off-by: pasta <pasta@dashboost.org>
@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.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants