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: Fixups to "Run all tests even if wallet is not compiled" #14179

Merged
merged 2 commits into from Sep 13, 2018

Conversation

Projects
None yet
5 participants
@MarcoFalke
Copy link
Member

commented Sep 9, 2018

Currently the functional tests require the wallet module to be compiled into the Bitcoin Core executable. For example the premine (or datadir cache) to speed up tests when run in parallel would mine a bunch of blocks and store the private keys to sign the coinbase tx outputs in a wallet. There is no need to have the overhead of the whole wallet module by using keys that are deterministic for all runs.

Note that this change most likely requires the ./test/cache/ to be cleared.

@MarcoFalke MarcoFalke added the Tests label Sep 9, 2018

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1809-qaPremine branch 2 times, most recently Sep 9, 2018

@Sjors

This comment has been minimized.

Copy link
Member

commented Sep 10, 2018

tACK fae497d on macOS 10.13.6

@jnewbery
Copy link
Member

left a comment

Concept ACK.

A couple of suggested changes inline. There are also a bunch of unrelated style changes in your commit. Can you remove those or split them into their own commit?

test/functional/test_framework/test_framework.py Outdated
@@ -247,6 +247,14 @@ def setup_nodes(self):
self.add_nodes(self.num_nodes, extra_args)
self.start_nodes()

def import_deterministic_coinbase_privkeys(self):
for n in self.nodes:
wallet_enabled = 'Wallet' in [line[3:-3] for line in n.help().splitlines() if line.startswith('==')]

This comment has been minimized.

Copy link
@jnewbery

jnewbery Sep 10, 2018

Member

This seems a little brittle (based on the exact text output of the help text).

Calling getwalletinfo and catching the returned error is an alternative way to do this.

test/functional/test_framework/test_framework.py Outdated
@@ -247,6 +247,14 @@ def setup_nodes(self):
self.add_nodes(self.num_nodes, extra_args)
self.start_nodes()

def import_deterministic_coinbase_privkeys(self):

This comment has been minimized.

Copy link
@jnewbery

jnewbery Sep 10, 2018

Member

Can you add a call to import_deterministic_coinbase_privkeys() to setup_nodes()? Would that reduce the number of tests that you need to change in this commit?

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Sep 10, 2018

Author Member

Good idea to call this by default... This is now always called before run_test, unless self.setup_clean_chain is set.

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1809-qaPremine branch 2 times, most recently to faa669c Sep 10, 2018

def get_deterministic_priv_key(self):
"""Return a deterministic priv key in base58, that only depends on the node's index"""
PRIV_KEYS = [
# adress , privkey

This comment has been minimized.

Copy link
@practicalswift

practicalswift Sep 11, 2018

Member

codespell:

test/functional/test_framework/test_node.py:103: adress  ==> address

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Sep 13, 2018

Author Member

Fixed typo

test/functional/wallet_listreceivedby.py Outdated
def import_deterministic_coinbase_privkeys(self):
assert_equal(0, len(self.nodes[1].listreceivedbyaddress(minconf=0, include_empty=True, include_watchonly=True)))
super().import_deterministic_coinbase_privkeys()
self.num_cb_reward_addresses = len(self.nodes[1].listreceivedbyaddress(minconf=0, include_empty=True, include_watchonly=True))

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Sep 12, 2018

Contributor

Maybe substitute cb_reward with just coinbase to be more consistent.

if len(comment) > 1:
addr_keypath = comment.split(" addr=")[1]
addr = addr_keypath.split(" ")[0]
key_date_label, comment = line.split("#")

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Sep 12, 2018

Contributor

Previous code and new changes here are pretty inscrutable. Can you explain at a high level what is changing here and why? Can you perhaps add comments to the code to make it clearer, for example to explain why new code skips lines containing 1970?

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Sep 13, 2018

Author Member

Added a comment. Note that it is easier to see that nothing changed with --ignore-all-space

@@ -168,6 +168,11 @@ def transact_and_mine(self, numblocks, mining_node):
newmem.append(utx)
self.memutxo = newmem

def import_deterministic_coinbase_privkeys(self):
self.start_nodes()

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Sep 12, 2018

Contributor

Couldn't the test framework implementation of import_deterministic_coinbase_privkeys be smart enough to know whether nodes are running or not with node.is_node_stopped (or other state)? It seems unfortunate that an individual test would need to override the import_deterministic_coinbase_privkeys method to get such basic functionality.

@@ -40,6 +40,13 @@ def set_test_params(self):
def setup_nodes(self):
skip_if_no_py3_zmq()
skip_if_no_bitcoind_zmq(self)

# Import keys

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Sep 12, 2018

Contributor

Why doesn't the default framework import_deterministic_coinbase_privkeys call work in this test? There should be a comment here to explain why this override is needed if the default behavior can't be fixed.

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Sep 13, 2018

Author Member

Removed the unneeded import_deterministic_coinbase_privkeys

@@ -116,10 +116,19 @@ def setup_network(self):
extra_args[i] += ["-prune=1"]

self.add_nodes(self.num_nodes, extra_args=extra_args)

# Import keys

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Sep 12, 2018

Contributor

Can you add comment explaining need for this override? The stop_nodes immediately followed by start_nodes below seems very odd. And given that this is a wallet test, maybe it would be simpler to just keep the old behavior and just use the wallet instead of generating to hardcoded addresses?

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Sep 13, 2018

Author Member

Added a comment

@MarcoFalke MarcoFalke changed the title qa: Premine to deterministic address with -disablewallet qa: Fixups to "Run all tests even if wallet is not compiled" Sep 13, 2018

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1809-qaPremine branch to fa8433e Sep 13, 2018

@ryanofsky
Copy link
Contributor

left a comment

utACK fa8433e, thanks for cleanup!

@MarcoFalke MarcoFalke merged commit fa8433e into bitcoin:master Sep 13, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit that referenced this pull request Sep 13, 2018

Merge #14179: qa: Fixups to "Run all tests even if wallet is not comp…
…iled"

fa8433e qa: Remove unneded import_deterministic_coinbase_privkeys overwrite, add comments (MarcoFalke)
e413c2d qa: Fix codespell error and have lint-spelling error instead of warn (MarcoFalke)

Pull request description:

  Currently the functional tests require the wallet module to be compiled into the Bitcoin Core executable. For example the premine (or datadir cache) to speed up tests when run in parallel would mine a bunch of blocks and store the private keys to sign the coinbase tx outputs in a wallet. There is no need to have the overhead of the whole wallet module by using keys that are deterministic for all runs.

  Note that this change most likely requires the `./test/cache/` to be cleared.

Tree-SHA512: 9ce26036b0e10f0f888f66a1e50be6a357343f9ffb302ae24a7bb3df2f083a31702ef308b738a03b08a1b623aeddac5d6563dc1b15078c0357b7dafad7808ec3

@MarcoFalke MarcoFalke deleted the MarcoFalke:Mf1809-qaPremine branch Sep 13, 2018

sipa added a commit that referenced this pull request Oct 17, 2018

Merge #14495: build: Warn (don't fail!) on spelling errors
c32cf6a Add ignored word: mut (practicalswift)
4ae50da Revert "qa: Fix codespell error and have lint-spelling error instead of warn" (practicalswift)

Pull request description:

  Revert `codespell` policy change introduced in #14179.

  Context: #13954 (comment)

Tree-SHA512: 4606b19bb32cdd661f90b3778759818d3493e5ed1a4a2f95982f07eeb6b9c889bc8d53cde31706e0a3b9524c3d3a7378f1b769a60baeb0d00da4c68fd3068114

@jnewbery jnewbery referenced this pull request Feb 26, 2019

Merged

test: Add .style.yapf #15479

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.