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

pull MarcoFalke wants to merge 2 commits into bitcoin:master from MarcoFalke:Mf1809-qaRunAllTheTests changing 86 files +502 −133

MarcoFalke commented at 5:08 PM on September 9, 2018: member

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.
MarcoFalke added the label Tests on Sep 9, 2018
MarcoFalke commented at 5:26 PM on September 9, 2018: member

Travis run for reference: https://travis-ci.org/bitcoin/bitcoin/jobs/426386820#L2937
DrahtBot commented at 8:30 PM on September 9, 2018: member
Note to reviewers: This pull request conflicts with the following ones:
- #13931 (Drop support for getrawtransaction for confirmed tx without txindex by sipa)
- #13922 (Lower default relay fees by ajtowns)
- #12246 (Bugfix: Only run bitcoin-tx tests when bitcoin-tx is enabled by luke-jr)
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 commented at 1:10 AM on September 10, 2018: contributor

This PR includes a commit that's also present in #14179 (fae497d). What's the relationship between the two PRs?
MarcoFalke commented at 1:19 AM on September 10, 2018: member

This is based on #14179, since that is a requirement.
Sjors commented at 8:41 AM on September 10, 2018: member
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?
MarcoFalke commented at 12:55 PM on September 10, 2018: member

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.

in test/functional/test_framework/test_framework.py:274 in fa2dad76d7 outdated

 263 | +        for n in self.nodes:
 264 | +            wallet_enabled = 'Wallet' in [line[3:-3] for line in n.help().splitlines() if line.startswith('==')]
 265 | +            if not wallet_enabled:
 266 | +                continue
 267 | +
 268 | +            n.importprivkey(n.get_deterministic_priv_key()[1])

Empact commented at 7:11 PM on September 10, 2018:

nit: could invert the conditional / drop the continue

MarcoFalke force-pushed on Sep 10, 2018
MarcoFalke force-pushed on Sep 10, 2018
qa: Premine to deterministic address with -disablewallet faa669cbcd
qa: Run all tests even if wallet is not compiled fac9539836
MarcoFalke force-pushed on Sep 10, 2018
laanwj commented at 7:40 AM on September 11, 2018: member

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

:+1:

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 commented at 1:48 PM on September 11, 2018: member

@laanwj the x86_64 Linux, No wallet Travis host already runs this: <img width="1286" alt="schermafbeelding 2018-09-11 om 15 47 39" src="https://user-images.githubusercontent.com/10217/45364293-0f5c3800-b5da-11e8-8034-fe4e62acd82a.png">
jnewbery commented at 8:51 PM on September 11, 2018: member

Concept ACK
laanwj commented at 9:35 AM on September 12, 2018: member

utACK
laanwj merged this on Sep 13, 2018
laanwj closed this on Sep 13, 2018
laanwj referenced this in commit 288ddf4ff5 on Sep 13, 2018
ryanofsky commented at 7:07 PM on September 13, 2018: member

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.
MarcoFalke referenced this in commit 7391dad1ea on Oct 4, 2018
MarcoFalke referenced this in commit fddbd82934 on Oct 4, 2018
MarcoFalke referenced this in commit b169519959 on Oct 4, 2018
MarcoFalke referenced this in commit 86fadee990 on Oct 25, 2018
MarcoFalke referenced this in commit f7adb32e38 on Oct 25, 2018
toxeus referenced this in commit 4fd800f89b on Nov 28, 2018
toxeus referenced this in commit bdcdded972 on Nov 28, 2018
PastaPastaPasta referenced this in commit 28d7ca8e69 on Jul 20, 2021
PastaPastaPasta referenced this in commit fcdf4a4092 on Jul 21, 2021
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels