From 234bd4463830e1dc82faa0adabb593aaeb0ad8d2 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Thu, 3 May 2018 14:44:14 +0200 Subject: [PATCH 01/12] pytest: Move wait_for to utils.py and make fund_channel a member Slowly moving towards a more pythonic approach to the testing code. Signed-off-by: Christian Decker --- tests/test_lightningd.py | 36 ++---------------------------------- tests/utils.py | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 34 deletions(-) diff --git a/tests/test_lightningd.py b/tests/test_lightningd.py index 4235631064c5..a2925f0113d9 100644 --- a/tests/test_lightningd.py +++ b/tests/test_lightningd.py @@ -1,5 +1,6 @@ from concurrent import futures from decimal import Decimal +from utils import wait_for import copy import json @@ -66,14 +67,6 @@ def setupBitcoind(directory): bitcoind.generate_block(1) -def wait_for(success, timeout=30, interval=0.1): - start_time = time.time() - while not success() and time.time() < start_time + timeout: - time.sleep(interval) - if time.time() > start_time + timeout: - raise ValueError("Error waiting for {}", success) - - def wait_forget_channels(node): """This node is closing all of its channels, check we are forgetting them """ @@ -307,31 +300,7 @@ def give_funds(self, l1, satoshi): # Returns the short channel-id: :: def fund_channel(self, l1, l2, amount): - # Generates a block, so we know next tx will be first in block. - self.give_funds(l1, amount + 1000000) - num_tx = len(l1.bitcoin.rpc.getrawmempool()) - - tx = l1.rpc.fundchannel(l2.info['id'], amount)['tx'] - # Technically, this is async to fundchannel. - l1.daemon.wait_for_log('sendrawtx exit 0') - - wait_for(lambda: len(l1.bitcoin.rpc.getrawmempool()) == num_tx + 1) - l1.bitcoin.generate_block(1) - # We wait until gossipd sees local update, as well as status NORMAL, - # so it can definitely route through. - l1.daemon.wait_for_logs(['update for channel .* now ACTIVE', 'to CHANNELD_NORMAL']) - l2.daemon.wait_for_logs(['update for channel .* now ACTIVE', 'to CHANNELD_NORMAL']) - - # Hacky way to find our output. - decoded = bitcoind.rpc.decoderawtransaction(tx) - for out in decoded['vout']: - # Sometimes a float? Sometimes a decimal? WTF Python?! - if out['scriptPubKey']['type'] == 'witness_v0_scripthash': - if out['value'] == Decimal(amount) / 10**8 or out['value'] * 10**8 == amount: - return "{}:1:{}".format(bitcoind.rpc.getblockcount(), out['n']) - # Intermittent decoding failure. See if it decodes badly twice? - decoded2 = bitcoind.rpc.decoderawtransaction(tx) - raise ValueError("Can't find {} payment in {} (1={} 2={})".format(amount, tx, decoded, decoded2)) + return l1.fund_channel(l2, amount) def pay(self, lsrc, ldst, amt, label=None, async=False): if not label: @@ -731,7 +700,6 @@ def test_reconnect_channel_peers(self): def test_balance(self): l1, l2 = self.connect() - self.fund_channel(l1, l2, 10**6) p1 = l1.rpc.getpeer(peer_id=l2.info['id'], level='info')['channels'][0] p2 = l2.rpc.getpeer(l1.info['id'], 'info')['channels'][0] diff --git a/tests/utils.py b/tests/utils.py index 4a4bf96d8d4f..a6090151f402 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -7,6 +7,7 @@ import time from bitcoin.rpc import RawProxy as BitcoinProxy +from decimal import Decimal BITCOIND_CONFIG = { @@ -28,6 +29,14 @@ DEVELOPER = os.getenv("DEVELOPER", "0") == "1" +def wait_for(success, timeout=30, interval=0.1): + start_time = time.time() + while not success() and time.time() < start_time + timeout: + time.sleep(interval) + if time.time() > start_time + timeout: + raise ValueError("Error waiting for {}", success) + + def write_config(filename, opts): with open(filename, 'w') as f: for k, v in opts.items(): @@ -425,3 +434,34 @@ def restart(self, timeout=10, clean=True): self.daemon.stop() self.daemon.start() + + def fund_channel(self, l2, amount): + + # Give yourself some funds to work with + addr = self.rpc.newaddr()['address'] + self.bitcoin.rpc.sendtoaddress(addr, (amount + 1000000) / 10**8) + numfunds = len(self.rpc.listfunds()['outputs']) + self.bitcoin.generate_block(1) + wait_for(lambda: len(self.rpc.listfunds()['outputs']) > numfunds) + + # Now go ahead and open a channel + num_tx = len(self.bitcoin.rpc.getrawmempool()) + tx = self.rpc.fundchannel(l2.info['id'], amount)['tx'] + + wait_for(lambda: len(self.bitcoin.rpc.getrawmempool()) == num_tx + 1) + self.bitcoin.generate_block(1) + # We wait until gossipd sees local update, as well as status NORMAL, + # so it can definitely route through. + self.daemon.wait_for_logs(['update for channel .* now ACTIVE', 'to CHANNELD_NORMAL']) + l2.daemon.wait_for_logs(['update for channel .* now ACTIVE', 'to CHANNELD_NORMAL']) + + # Hacky way to find our output. + decoded = self.bitcoin.rpc.decoderawtransaction(tx) + for out in decoded['vout']: + # Sometimes a float? Sometimes a decimal? WTF Python?! + if out['scriptPubKey']['type'] == 'witness_v0_scripthash': + if out['value'] == Decimal(amount) / 10**8 or out['value'] * 10**8 == amount: + return "{}:1:{}".format(self.bitcoin.rpc.getblockcount(), out['n']) + # Intermittent decoding failure. See if it decodes badly twice? + decoded2 = self.bitcoin.rpc.decoderawtransaction(tx) + raise ValueError("Can't find {} payment in {} (1={} 2={})".format(amount, tx, decoded, decoded2)) From 9a8c50db8fb7e7643d0fb1056f59de2b7dd58ecb Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Thu, 3 May 2018 16:00:53 +0200 Subject: [PATCH 02/12] pytest: Add helper to start a number of nodes in parallel Especially with valgrind this allows us to safe quite some time on multi-core machines since startup is about the most expensive operation in the tests. In a simple test spinning up 3 nodes this gave me about 25% - 30% test time reduction. The effects will be smaller for single core machines, hoping Travis handles these gracefully. Signed-off-by: Christian Decker --- tests/test_lightningd.py | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/tests/test_lightningd.py b/tests/test_lightningd.py index a2925f0113d9..364ecc3173eb 100644 --- a/tests/test_lightningd.py +++ b/tests/test_lightningd.py @@ -102,9 +102,46 @@ def __init__(self, testname, bitcoind, executor): self.executor = executor self.bitcoind = bitcoind + def split_options(self, opts): + """Split node options from cli options + + Some options are used to instrument the node wrapper and some are passed + to the daemon on the command line. Split them so we know where to use + them. + """ + node_opt_keys = [ + 'disconnect', + 'may_fail', + 'may_reconnect', + 'random_hsm', + 'fake_bitcoin_cli' + ] + node_opts = {k: v for k, v in opts.items() if k in node_opt_keys} + cli_opts = {k: v for k, v in opts.items() if k not in node_opt_keys} + return node_opts, cli_opts + def get_next_port(self): return 16330 + self.next_id + def get_nodes(self, num_nodes, opts=None): + """Start a number of nodes in parallel, each with its own options + """ + if opts is None: + # No opts were passed in, give some dummy opts + opts = [{} for _ in range(num_nodes)] + elif isinstance(opts, dict): + # A single dict was passed in, so we use these opts for all nodes + opts = [opts] * num_nodes + + assert len(opts) == num_nodes + + jobs = [] + for i in range(num_nodes): + node_opts, cli_opts = self.split_options(opts[i]) + jobs.append(self.executor.submit(self.get_node, options=cli_opts, **node_opts)) + + return [j.result() for j in jobs] + def get_node(self, disconnect=None, options=None, may_fail=False, may_reconnect=False, random_hsm=False, fake_bitcoin_cli=False): node_id = self.next_id port = self.get_next_port() From 37c856d73dcc49a9d52946b7a898c3d148ea57c2 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Thu, 3 May 2018 17:00:46 +0200 Subject: [PATCH 03/12] pytest: Allow stdout to be dropped for bitcoind We never really look at the output, and it's rather noisy, so we just stop writing to the log. Signed-off-by: Christian Decker --- tests/utils.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/tests/utils.py b/tests/utils.py index a6090151f402..155787233220 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -50,7 +50,7 @@ class TailableProc(object): tail the processes and react to their output. """ - def __init__(self, outputDir=None): + def __init__(self, outputDir=None, verbose=True): self.logs = [] self.logs_cond = threading.Condition(threading.RLock()) self.env = os.environ @@ -59,6 +59,9 @@ def __init__(self, outputDir=None): self.outputDir = outputDir self.logsearch_start = 0 + # Should we be logging lines we read from stdout? + self.verbose = verbose + def start(self): """Start the underlying process and start monitoring it. """ @@ -110,9 +113,10 @@ def tail(self): for line in iter(self.proc.stdout.readline, ''): if len(line) == 0: break + if self.verbose: + logging.debug("%s: %s", self.prefix, line.decode().rstrip()) with self.logs_cond: self.logs.append(str(line.rstrip())) - logging.debug("%s: %s", self.prefix, line.decode().rstrip()) self.logs_cond.notifyAll() self.running = False self.proc.stdout.close() @@ -206,7 +210,7 @@ def __getattr__(self, name): class BitcoinD(TailableProc): def __init__(self, bitcoin_dir="/tmp/bitcoind-test", rpcport=18332): - TailableProc.__init__(self, bitcoin_dir) + TailableProc.__init__(self, bitcoin_dir, verbose=False) self.bitcoin_dir = bitcoin_dir self.rpcport = rpcport From 2b0dadaf5a5321844af7edc242d1c56dbeac8131 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Thu, 3 May 2018 22:28:39 +0200 Subject: [PATCH 04/12] pytest: Let `connect` start nodes in parallel Signed-off-by: Christian Decker --- tests/test_lightningd.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/test_lightningd.py b/tests/test_lightningd.py index 364ecc3173eb..f45c42495a34 100644 --- a/tests/test_lightningd.py +++ b/tests/test_lightningd.py @@ -316,8 +316,7 @@ def tearDown(self): class LightningDTests(BaseLightningDTests): def connect(self, may_reconnect=False): - l1 = self.node_factory.get_node(may_reconnect=may_reconnect) - l2 = self.node_factory.get_node(may_reconnect=may_reconnect) + l1, l2 = self.node_factory.get_nodes(2, opts={'may_reconnect': may_reconnect}) ret = l1.rpc.connect(l2.info['id'], 'localhost', l2.info['port']) assert ret['id'] == l2.info['id'] From 9e5ad82f6378a1b8ce0e05b90ec8b108fb2fce4c Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Fri, 4 May 2018 11:59:21 +0200 Subject: [PATCH 05/12] pytest: Add py.test fixtures and migrate first example test This is the first example of the py.test style fixtures which should allow us to write much cleaner and nicer tests. Signed-off-by: Christian Decker --- Makefile | 5 +- tests/fixtures.py | 144 +++++++++++++++++++++++++++++++++++++++ tests/test_gossip.py | 60 ++++++++++++++++ tests/test_lightningd.py | 52 -------------- 4 files changed, 207 insertions(+), 54 deletions(-) create mode 100644 tests/fixtures.py create mode 100644 tests/test_gossip.py diff --git a/Makefile b/Makefile index b0bfb1c58301..cf535fefec8d 100644 --- a/Makefile +++ b/Makefile @@ -203,9 +203,10 @@ check: pytest: $(ALL_PROGRAMS) ifndef PYTEST - PYTHONPATH=contrib/pylightning:$$PYTHONPATH DEVELOPER=$(DEVELOPER) python3 tests/test_lightningd.py -f + @echo "py.test is required to run the integration tests, please install using 'pip3 install -r tests/requirements.txt'" + exit 1 else - PYTHONPATH=contrib/pylightning:$$PYTHONPATH TEST_DEBUG=1 DEVELOPER=$(DEVELOPER) $(PYTEST) -vx tests/test_lightningd.py --test-group=$(TEST_GROUP) --test-group-count=$(TEST_GROUP_COUNT) $(PYTEST_OPTS) + PYTHONPATH=contrib/pylightning:$$PYTHONPATH TEST_DEBUG=1 DEVELOPER=$(DEVELOPER) $(PYTEST) -vx tests/ --test-group=$(TEST_GROUP) --test-group-count=$(TEST_GROUP_COUNT) $(PYTEST_OPTS) endif # Keep includes in alpha order. diff --git a/tests/fixtures.py b/tests/fixtures.py new file mode 100644 index 000000000000..cf2e679a9c37 --- /dev/null +++ b/tests/fixtures.py @@ -0,0 +1,144 @@ +from concurrent import futures +from test_lightningd import NodeFactory + +import logging +import os +import pytest +import re +import tempfile +import utils + + +TEST_DIR = tempfile.mkdtemp(prefix='ltests-') +VALGRIND = os.getenv("NO_VALGRIND", "0") == "0" +DEVELOPER = os.getenv("DEVELOPER", "0") == "1" +TEST_DEBUG = os.getenv("TEST_DEBUG", "0") == "1" + + +@pytest.fixture +def directory(test_name): + """Return a per-test specific directory + """ + global TEST_DIR + yield os.path.join(TEST_DIR, test_name) + + +@pytest.fixture +def test_name(request): + yield request.function.__name__ + + +@pytest.fixture +def bitcoind(directory): + bitcoind = utils.BitcoinD(bitcoin_dir=directory, rpcport=28332) + try: + bitcoind.start() + except Exception: + bitcoind.stop() + raise + + info = bitcoind.rpc.getnetworkinfo() + + if info['version'] < 160000: + bitcoind.rpc.stop() + raise ValueError("bitcoind is too old. At least version 16000 (v0.16.0)" + " is needed, current version is {}".format(info['version'])) + + info = bitcoind.rpc.getblockchaininfo() + # Make sure we have some spendable funds + if info['blocks'] < 101: + bitcoind.generate_block(101 - info['blocks']) + elif bitcoind.rpc.getwalletinfo()['balance'] < 1: + logging.debug("Insufficient balance, generating 1 block") + bitcoind.generate_block(1) + + yield bitcoind + + try: + bitcoind.rpc.stop() + except Exception: + bitcoind.proc.kill() + bitcoind.proc.wait() + + +@pytest.fixture +def node_factory(directory, test_name, bitcoind, executor): + nf = NodeFactory(test_name, bitcoind, executor, directory=directory) + yield nf + err_count = 0 + ok = nf.killall([not n.may_fail for n in nf.nodes]) + if VALGRIND: + for node in nf.nodes: + err_count += printValgrindErrors(node) + if err_count: + raise ValueError("{} nodes reported valgrind errors".format(err_count)) + + for node in nf.nodes: + err_count += printCrashLog(node) + if err_count: + raise ValueError("{} nodes had crash.log files".format(err_count)) + for node in nf.nodes: + err_count += checkReconnect(node) + if err_count: + raise ValueError("{} nodes had unexpected reconnections".format(err_count)) + + if not ok: + raise Exception("At least one lightning exited with unexpected non-zero return code") + + +def getValgrindErrors(node): + for error_file in os.listdir(node.daemon.lightning_dir): + if not re.fullmatch("valgrind-errors.\d+", error_file): + continue + with open(os.path.join(node.daemon.lightning_dir, error_file), 'r') as f: + errors = f.read().strip() + if errors: + return errors, error_file + return None, None + + +def printValgrindErrors(node): + errors, fname = getValgrindErrors(node) + if errors: + print("-" * 31, "Valgrind errors", "-" * 32) + print("Valgrind error file:", fname) + print(errors) + print("-" * 80) + return 1 if errors else 0 + + +def getCrashLog(node): + if node.may_fail: + return None, None + try: + crashlog = os.path.join(node.daemon.lightning_dir, 'crash.log') + with open(crashlog, 'r') as f: + return f.readlines(), crashlog + except Exception: + return None, None + + +def printCrashLog(node): + errors, fname = getCrashLog(node) + if errors: + print("-" * 10, "{} (last 50 lines)".format(fname), "-" * 10) + for l in errors[-50:]: + print(l, end='') + print("-" * 80) + return 1 if errors else 0 + + +def checkReconnect(node): + # Without DEVELOPER, we can't suppress reconnection. + if node.may_reconnect or not DEVELOPER: + return 0 + if node.daemon.is_in_log('Peer has reconnected'): + return 1 + return 0 + + +@pytest.fixture +def executor(): + ex = futures.ThreadPoolExecutor(max_workers=20) + yield ex + ex.shutdown(wait=False) diff --git a/tests/test_gossip.py b/tests/test_gossip.py new file mode 100644 index 000000000000..963cc183cd90 --- /dev/null +++ b/tests/test_gossip.py @@ -0,0 +1,60 @@ +from fixtures import * # noqa: F401,F403 +from test_lightningd import wait_for + +import os +import time +import unittest + + +DEVELOPER = os.getenv("DEVELOPER", "0") == "1" + + +@unittest.skipIf(not DEVELOPER, "needs DEVELOPER=1 for --dev-broadcast-interval") +def test_gossip_pruning(node_factory, bitcoind): + """ Create channel and see it being updated in time before pruning + """ + opts = {'channel-update-interval': 5} + l1, l2, l3 = node_factory.get_nodes(3, opts) + + l1.rpc.connect(l2.info['id'], 'localhost', l2.info['port']) + l2.rpc.connect(l3.info['id'], 'localhost', l3.info['port']) + + scid1 = l1.fund_channel(l2, 10**6) + scid2 = l2.fund_channel(l3, 10**6) + + bitcoind.rpc.generate(6) + + # Channels should be activated locally + wait_for(lambda: [c['active'] for c in l1.rpc.listchannels()['channels']] == [True] * 4) + wait_for(lambda: [c['active'] for c in l2.rpc.listchannels()['channels']] == [True] * 4) + wait_for(lambda: [c['active'] for c in l3.rpc.listchannels()['channels']] == [True] * 4) + + # All of them should send a keepalive message + l1.daemon.wait_for_logs([ + 'Sending keepalive channel_update for {}'.format(scid1), + ]) + l2.daemon.wait_for_logs([ + 'Sending keepalive channel_update for {}'.format(scid1), + 'Sending keepalive channel_update for {}'.format(scid2), + ]) + l3.daemon.wait_for_logs([ + 'Sending keepalive channel_update for {}'.format(scid2), + ]) + + # Now kill l3, so that l2 and l1 can prune it from their view after 10 seconds + + # FIXME: This sleep() masks a real bug: that channeld sends a + # channel_update message (to disable the channel) with same + # timestamp as the last keepalive, and thus is ignored. The minimal + # fix is to backdate the keepalives 1 second, but maybe we should + # simply have gossipd generate all updates? + time.sleep(1) + l3.stop() + + l1.daemon.wait_for_log("Pruning channel {} from network view".format(scid2)) + l2.daemon.wait_for_log("Pruning channel {} from network view".format(scid2)) + + assert scid2 not in [c['short_channel_id'] for c in l1.rpc.listchannels()['channels']] + assert scid2 not in [c['short_channel_id'] for c in l2.rpc.listchannels()['channels']] + assert l3.info['id'] not in [n['nodeid'] for n in l1.rpc.listnodes()['nodes']] + assert l3.info['id'] not in [n['nodeid'] for n in l2.rpc.listnodes()['nodes']] diff --git a/tests/test_lightningd.py b/tests/test_lightningd.py index f45c42495a34..59ac0cb5914d 100644 --- a/tests/test_lightningd.py +++ b/tests/test_lightningd.py @@ -2440,58 +2440,6 @@ def test_gossip_weirdalias(self): node = l2.rpc.listnodes(l1.info['id'])['nodes'][0] assert node['alias'] == weird_name - @unittest.skipIf(not DEVELOPER, "needs DEVELOPER=1 for --dev-broadcast-interval") - def test_gossip_pruning(self): - """ Create channel and see it being updated in time before pruning - """ - opts = {'channel-update-interval': 5} - l1 = self.node_factory.get_node(options=opts) - l2 = self.node_factory.get_node(options=opts) - l3 = self.node_factory.get_node(options=opts) - - l1.rpc.connect(l2.info['id'], 'localhost', l2.info['port']) - l2.rpc.connect(l3.info['id'], 'localhost', l3.info['port']) - - scid1 = self.fund_channel(l1, l2, 10**6) - scid2 = self.fund_channel(l2, l3, 10**6) - - l1.bitcoin.rpc.generate(6) - - # Channels should be activated locally - wait_for(lambda: [c['active'] for c in l1.rpc.listchannels()['channels']] == [True] * 4) - wait_for(lambda: [c['active'] for c in l2.rpc.listchannels()['channels']] == [True] * 4) - wait_for(lambda: [c['active'] for c in l3.rpc.listchannels()['channels']] == [True] * 4) - - # All of them should send a keepalive message - l1.daemon.wait_for_logs([ - 'Sending keepalive channel_update for {}'.format(scid1), - ]) - l2.daemon.wait_for_logs([ - 'Sending keepalive channel_update for {}'.format(scid1), - 'Sending keepalive channel_update for {}'.format(scid2), - ]) - l3.daemon.wait_for_logs([ - 'Sending keepalive channel_update for {}'.format(scid2), - ]) - - # Now kill l3, so that l2 and l1 can prune it from their view after 10 seconds - - # FIXME: This sleep() masks a real bug: that channeld sends a - # channel_update message (to disable the channel) with same - # timestamp as the last keepalive, and thus is ignored. The minimal - # fix is to backdate the keepalives 1 second, but maybe we should - # simply have gossipd generate all updates? - time.sleep(1) - l3.stop() - - l1.daemon.wait_for_log("Pruning channel {} from network view".format(scid2)) - l2.daemon.wait_for_log("Pruning channel {} from network view".format(scid2)) - - assert scid2 not in [c['short_channel_id'] for c in l1.rpc.listchannels()['channels']] - assert scid2 not in [c['short_channel_id'] for c in l2.rpc.listchannels()['channels']] - assert l3.info['id'] not in [n['nodeid'] for n in l1.rpc.listnodes()['nodes']] - assert l3.info['id'] not in [n['nodeid'] for n in l2.rpc.listnodes()['nodes']] - @unittest.skipIf(not DEVELOPER, "needs DEVELOPER=1 for --dev-no-reconnect") def test_gossip_persistence(self): """Gossip for a while, restart and it should remember. From 643ce6136f87ecb41223c6b6a25aadaf4ac3636b Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Fri, 4 May 2018 18:27:07 +0200 Subject: [PATCH 06/12] pytest: Make the directory of the NodeFactory configurable Signed-off-by: Christian Decker --- tests/test_lightningd.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/tests/test_lightningd.py b/tests/test_lightningd.py index 59ac0cb5914d..671273e1be2a 100644 --- a/tests/test_lightningd.py +++ b/tests/test_lightningd.py @@ -95,12 +95,17 @@ def teardown_bitcoind(): class NodeFactory(object): """A factory to setup and start `lightningd` daemons. """ - def __init__(self, testname, bitcoind, executor): + def __init__(self, testname, bitcoind, executor, directory=None): self.testname = testname self.next_id = 1 self.nodes = [] self.executor = executor self.bitcoind = bitcoind + if directory is not None: + self.directory = directory + else: + self.directory = os.path.join(TEST_DIR, testname) + self.lock = threading.Lock() def split_options(self, opts): """Split node options from cli options @@ -148,7 +153,7 @@ def get_node(self, disconnect=None, options=None, may_fail=False, may_reconnect= self.next_id += 1 lightning_dir = os.path.join( - TEST_DIR, self.testname, "lightning-{}/".format(node_id)) + self.directory, "lightning-{}/".format(node_id)) if os.path.exists(lightning_dir): shutil.rmtree(lightning_dir) From 93b4482fdf232d9edb2f9604d667387d832c8e8e Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Fri, 4 May 2018 19:19:44 +0200 Subject: [PATCH 07/12] pytest: Use random ports for bitcoind and lightningd to allow parallel testing Adds a new dependency, but totally worth it :-) Signed-off-by: Christian Decker --- tests/fixtures.py | 2 +- tests/test_lightningd.py | 15 ++++++++------- tests/utils.py | 6 +++++- 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/tests/fixtures.py b/tests/fixtures.py index cf2e679a9c37..d00635c4aab8 100644 --- a/tests/fixtures.py +++ b/tests/fixtures.py @@ -30,7 +30,7 @@ def test_name(request): @pytest.fixture def bitcoind(directory): - bitcoind = utils.BitcoinD(bitcoin_dir=directory, rpcport=28332) + bitcoind = utils.BitcoinD(bitcoin_dir=directory, rpcport=None) try: bitcoind.start() except Exception: diff --git a/tests/test_lightningd.py b/tests/test_lightningd.py index 671273e1be2a..4730749d46ef 100644 --- a/tests/test_lightningd.py +++ b/tests/test_lightningd.py @@ -1,5 +1,6 @@ from concurrent import futures from decimal import Decimal +from ephemeral_port_reserve import reserve as reserve_port from utils import wait_for import copy @@ -43,7 +44,7 @@ def to_json(arg): def setupBitcoind(directory): global bitcoind - bitcoind = utils.BitcoinD(bitcoin_dir=directory, rpcport=28332) + bitcoind = utils.BitcoinD(bitcoin_dir=directory, rpcport=None) try: bitcoind.start() @@ -126,7 +127,8 @@ def split_options(self, opts): return node_opts, cli_opts def get_next_port(self): - return 16330 + self.next_id + with self.lock: + return reserve_port() def get_nodes(self, num_nodes, opts=None): """Start a number of nodes in parallel, each with its own options @@ -148,9 +150,10 @@ def get_nodes(self, num_nodes, opts=None): return [j.result() for j in jobs] def get_node(self, disconnect=None, options=None, may_fail=False, may_reconnect=False, random_hsm=False, fake_bitcoin_cli=False): - node_id = self.next_id + with self.lock: + node_id = self.next_id + self.next_id += 1 port = self.get_next_port() - self.next_id += 1 lightning_dir = os.path.join( self.directory, "lightning-{}/".format(node_id)) @@ -662,9 +665,7 @@ def test_connect_by_gossip(self): """ l1 = self.node_factory.get_node() l2 = self.node_factory.get_node() - # Force l3 to give its address. - l3port = self.node_factory.get_next_port() - l3 = self.node_factory.get_node(options={"ipaddr": "127.0.0.1:{}".format(l3port)}) + l3 = self.node_factory.get_node(options={"ipaddr": "127.0.0.1"}) l2.rpc.connect(l3.info['id'], 'localhost', l3.info['port']) diff --git a/tests/utils.py b/tests/utils.py index 155787233220..bc2ba49153d9 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -8,6 +8,7 @@ from bitcoin.rpc import RawProxy as BitcoinProxy from decimal import Decimal +from ephemeral_port_reserve import reserve BITCOIND_CONFIG = { @@ -209,9 +210,12 @@ def __getattr__(self, name): class BitcoinD(TailableProc): - def __init__(self, bitcoin_dir="/tmp/bitcoind-test", rpcport=18332): + def __init__(self, bitcoin_dir="/tmp/bitcoind-test", rpcport=None): TailableProc.__init__(self, bitcoin_dir, verbose=False) + if rpcport is None: + rpcport = reserve() + self.bitcoin_dir = bitcoin_dir self.rpcport = rpcport self.prefix = 'bitcoind' From a58c08eff1f112356e37c2180c165a2d97c87bf4 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Fri, 4 May 2018 19:40:51 +0200 Subject: [PATCH 08/12] builder: Add the ephemeral-port-reserve dependency to the builder Signed-off-by: Christian Decker --- contrib/Dockerfile.builder | 2 +- contrib/Dockerfile.builder.fedora | 4 ++-- contrib/Dockerfile.builder.i386 | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/contrib/Dockerfile.builder b/contrib/Dockerfile.builder index b6918b546e09..5211f4d99f99 100644 --- a/contrib/Dockerfile.builder +++ b/contrib/Dockerfile.builder @@ -47,4 +47,4 @@ RUN cd /tmp/ && \ rm -rf bitcoin.tar.gz /tmp/bitcoin-$BITCOIN_VERSION RUN pip3 install --upgrade pip && \ - python3 -m pip install python-bitcoinlib==0.7.0 pytest==3.0.5 setuptools==36.6.0 pytest-test-groups==1.0.3 flake8==3.5.0 pytest-rerunfailures==3.1 + python3 -m pip install python-bitcoinlib==0.7.0 pytest==3.0.5 setuptools==36.6.0 pytest-test-groups==1.0.3 flake8==3.5.0 pytest-rerunfailures==3.1 ephemeral-port-reserve==1.1.0 pytest-xdist==1.22.2 diff --git a/contrib/Dockerfile.builder.fedora b/contrib/Dockerfile.builder.fedora index bcfea7423cdf..91ab2c71ccf4 100644 --- a/contrib/Dockerfile.builder.fedora +++ b/contrib/Dockerfile.builder.fedora @@ -25,5 +25,5 @@ RUN wget https://bitcoin.org/bin/bitcoin-core-$BITCOIN_VERSION/bitcoin-$BITCOIN_ mv bitcoin-$BITCOIN_VERSION/bin/bitcoin* /usr/local/bin/ && \ rm -rf bitcoin.tar.gz bitcoin-$BITCOIN_VERSION -RUN pip3 install --upgrade pip && \ - pip3 install python-bitcoinlib==0.7.0 pytest==3.0.5 setuptools==36.6.0 pytest-test-groups==1.0.3 flake8==3.5.0 pytest-rerunfailures==3.1 +RUN python3 -m pip install --upgrade pip && \ + python3 -m pip install python-bitcoinlib==0.7.0 pytest==3.0.5 setuptools==36.6.0 pytest-test-groups==1.0.3 flake8==3.5.0 pytest-rerunfailures==3.1 ephemeral-port-reserve==1.1.0 diff --git a/contrib/Dockerfile.builder.i386 b/contrib/Dockerfile.builder.i386 index fe0f5087184c..9d0cd270824b 100644 --- a/contrib/Dockerfile.builder.i386 +++ b/contrib/Dockerfile.builder.i386 @@ -47,4 +47,4 @@ RUN cd /tmp/ && \ rm -rf bitcoin.tar.gz /tmp/bitcoin-$BITCOIN_VERSION RUN pip3 install --upgrade pip && \ - python3 -m pip install python-bitcoinlib==0.7.0 pytest==3.0.5 setuptools==36.6.0 pytest-test-groups==1.0.3 flake8==3.5.0 pytest-rerunfailures==3.1 + python3 -m pip install python-bitcoinlib==0.7.0 pytest==3.0.5 setuptools==36.6.0 pytest-test-groups==1.0.3 flake8==3.5.0 pytest-rerunfailures==3.1 ephemeral-port-reserve==1.1.0 pytest-xdist==1.22.2 From e644888f49498ca8021ae5997a8f9ceaae28855f Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Sat, 5 May 2018 15:30:14 +0200 Subject: [PATCH 09/12] pytest: Ensure unique test directories per test even if rerun We add an attempt number to the test directory to improve the test-isolation and allow for multiple reruns of the same test, without re-using any of the lightning-dirs or bitcoin-datadirs. Signed-off-by: Christian Decker --- tests/fixtures.py | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/tests/fixtures.py b/tests/fixtures.py index d00635c4aab8..5b939e920b24 100644 --- a/tests/fixtures.py +++ b/tests/fixtures.py @@ -15,12 +15,22 @@ TEST_DEBUG = os.getenv("TEST_DEBUG", "0") == "1" +# A dict in which we count how often a particular test has run so far. Used to +# give each attempt its own numbered directory, and avoid clashes. +__attempts = {} + + @pytest.fixture def directory(test_name): - """Return a per-test specific directory + """Return a per-test specific directory. + + This makes a unique test-directory even if a test is rerun multiple times. + """ - global TEST_DIR - yield os.path.join(TEST_DIR, test_name) + global TEST_DIR, __attempts + # Auto set value if it isn't in the dict yet + __attempts[test_name] = __attempts.get(test_name, 0) + 1 + yield os.path.join(TEST_DIR, "{}_{}".format(test_name, __attempts[test_name])) @pytest.fixture From 85294d5a6dc73039adf4bb2e049b7a07ecc13b75 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Sat, 5 May 2018 16:45:09 +0200 Subject: [PATCH 10/12] contrib: Add the flaky dependency to the builder image Signed-off-by: Christian Decker --- contrib/Dockerfile.builder | 2 +- contrib/Dockerfile.builder.i386 | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/contrib/Dockerfile.builder b/contrib/Dockerfile.builder index 5211f4d99f99..ba1f901a9f49 100644 --- a/contrib/Dockerfile.builder +++ b/contrib/Dockerfile.builder @@ -47,4 +47,4 @@ RUN cd /tmp/ && \ rm -rf bitcoin.tar.gz /tmp/bitcoin-$BITCOIN_VERSION RUN pip3 install --upgrade pip && \ - python3 -m pip install python-bitcoinlib==0.7.0 pytest==3.0.5 setuptools==36.6.0 pytest-test-groups==1.0.3 flake8==3.5.0 pytest-rerunfailures==3.1 ephemeral-port-reserve==1.1.0 pytest-xdist==1.22.2 + python3 -m pip install python-bitcoinlib==0.7.0 pytest==3.0.5 setuptools==36.6.0 pytest-test-groups==1.0.3 flake8==3.5.0 pytest-rerunfailures==3.1 ephemeral-port-reserve==1.1.0 pytest-xdist==1.22.2 flaky==3.4.0 diff --git a/contrib/Dockerfile.builder.i386 b/contrib/Dockerfile.builder.i386 index 9d0cd270824b..b425b47b8eee 100644 --- a/contrib/Dockerfile.builder.i386 +++ b/contrib/Dockerfile.builder.i386 @@ -47,4 +47,4 @@ RUN cd /tmp/ && \ rm -rf bitcoin.tar.gz /tmp/bitcoin-$BITCOIN_VERSION RUN pip3 install --upgrade pip && \ - python3 -m pip install python-bitcoinlib==0.7.0 pytest==3.0.5 setuptools==36.6.0 pytest-test-groups==1.0.3 flake8==3.5.0 pytest-rerunfailures==3.1 ephemeral-port-reserve==1.1.0 pytest-xdist==1.22.2 + python3 -m pip install python-bitcoinlib==0.7.0 pytest==3.0.5 setuptools==36.6.0 pytest-test-groups==1.0.3 flake8==3.5.0 pytest-rerunfailures==3.1 ephemeral-port-reserve==1.1.0 pytest-xdist==1.22.2 flaky==3.4.0 From ba3647279cf96ef40fc9c8f681eafd9129afb349 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Sat, 5 May 2018 16:45:36 +0200 Subject: [PATCH 11/12] pytest: Add flaky dependency and mark as test_closing_different_fees This is mainly just a stopgap solution until we get to stabilize the test. Signed-off-by: Christian Decker --- tests/requirements.txt | 3 +++ tests/test_lightningd.py | 2 ++ 2 files changed, 5 insertions(+) diff --git a/tests/requirements.txt b/tests/requirements.txt index 5928dec47139..17e6d7bc8efb 100644 --- a/tests/requirements.txt +++ b/tests/requirements.txt @@ -1 +1,4 @@ python-bitcoinlib==0.7.0 +ephemeral-port-reserve==1.1.0 +pytest-forked==0.2 +flaky==3.4.0 diff --git a/tests/test_lightningd.py b/tests/test_lightningd.py index 4730749d46ef..2452723c6f32 100644 --- a/tests/test_lightningd.py +++ b/tests/test_lightningd.py @@ -1,6 +1,7 @@ from concurrent import futures from decimal import Decimal from ephemeral_port_reserve import reserve as reserve_port +from flaky import flaky from utils import wait_for import copy @@ -1437,6 +1438,7 @@ def test_bitcoin_failure(self): bitcoind.generate_block(5) sync_blockheight([l1]) + @flaky def test_closing_different_fees(self): l1 = self.node_factory.get_node() From 07f2e7d3b7eea815e60b7a7958e2ffcc9aa1506d Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Sat, 5 May 2018 17:15:53 +0200 Subject: [PATCH 12/12] pytest: Give lightningd nodes a numeric ID to prefix logs This used to be the port, but since we no longer have fixed ports, and we start them in random order we can't easily distinguish them by the port anymore. Just use a numeric ID that matches their lightning-dirs. Signed-off-by: Christian Decker --- tests/test_lightningd.py | 5 ++++- tests/utils.py | 4 ++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/test_lightningd.py b/tests/test_lightningd.py index 2452723c6f32..cdb86b548c24 100644 --- a/tests/test_lightningd.py +++ b/tests/test_lightningd.py @@ -163,7 +163,10 @@ def get_node(self, disconnect=None, options=None, may_fail=False, may_reconnect= shutil.rmtree(lightning_dir) socket_path = os.path.join(lightning_dir, "lightning-rpc").format(node_id) - daemon = utils.LightningD(lightning_dir, self.bitcoind.bitcoin_dir, port=port, random_hsm=random_hsm) + daemon = utils.LightningD( + lightning_dir, self.bitcoind.bitcoin_dir, + port=port, random_hsm=random_hsm, node_id=node_id + ) # If we have a disconnect string, dump it to a file for daemon. if disconnect: with open(os.path.join(lightning_dir, "dev_disconnect"), "w") as f: diff --git a/tests/utils.py b/tests/utils.py index bc2ba49153d9..25e5c88c9349 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -251,7 +251,7 @@ def generate_block(self, numblocks=1): class LightningD(TailableProc): - def __init__(self, lightning_dir, bitcoin_dir, port=9735, random_hsm=False): + def __init__(self, lightning_dir, bitcoin_dir, port=9735, random_hsm=False, node_id=0): TailableProc.__init__(self, lightning_dir) self.lightning_dir = lightning_dir self.port = port @@ -281,7 +281,7 @@ def __init__(self, lightning_dir, bitcoin_dir, port=9735, random_hsm=False): f.write(seed) if DEVELOPER: self.opts['dev-broadcast-interval'] = 1000 - self.prefix = 'lightningd(%d)' % (port) + self.prefix = 'lightningd-%d' % (node_id) @property def cmd_line(self):