From 4cada557ba8dfeb4ba7f0220d68e91ec2bb5c8b1 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 21 Jul 2022 14:09:30 +0930 Subject: [PATCH] pytest: don't redirect stderr by default. Some tests need to inspect it, but most don't, and I suspect I'm missing some error messages due to this. Signed-off-by: Rusty Russell --- contrib/pyln-testing/pyln/testing/utils.py | 34 +++++++++++++--------- tests/test_db.py | 4 +-- tests/test_gossip.py | 3 +- tests/test_misc.py | 8 ++--- tests/test_plugin.py | 12 ++++---- tests/test_wallet.py | 2 +- 6 files changed, 35 insertions(+), 28 deletions(-) diff --git a/contrib/pyln-testing/pyln/testing/utils.py b/contrib/pyln-testing/pyln/testing/utils.py index e9d903995..5ca70e01a 100644 --- a/contrib/pyln-testing/pyln/testing/utils.py +++ b/contrib/pyln-testing/pyln/testing/utils.py @@ -201,7 +201,7 @@ class TailableProc(object): # pass it to the log matcher and not print it to stdout). self.log_filter = lambda line: False - def start(self, stdin=None, stdout_redir=True): + def start(self, stdin=None, stdout_redir=True, stderr_redir=True): """Start the underlying process and start monitoring it. If stdout_redir is false, you have to make sure logs go into outputDir/log @@ -209,16 +209,21 @@ class TailableProc(object): """ logging.debug("Starting '%s'", " ".join(self.cmd_line)) if stdout_redir: - self.proc = subprocess.Popen(self.cmd_line, - stdin=stdin, - stdout=self.stdout_write, - stderr=self.stderr_write, - env=self.env) + stdout = self.stdout_write else: - self.proc = subprocess.Popen(self.cmd_line, - stdin=stdin, - stderr=self.stderr_write, - env=self.env) + stdout = None + if stderr_redir: + stderr = self.stderr_write + self.stderr_redir = True + else: + stderr = None + self.stderr_redir = False + + self.proc = subprocess.Popen(self.cmd_line, + stdin=stdin, + stdout=stdout, + stderr=stderr, + env=self.env) def stop(self, timeout=10): self.proc.terminate() @@ -268,6 +273,7 @@ class TailableProc(object): def is_in_stderr(self, regex): """Look for `regex` in stderr.""" + assert self.stderr_redir self.logs_catchup() ex = re.compile(regex) for l in self.err_logs: @@ -618,9 +624,9 @@ class LightningD(TailableProc): return self.cmd_prefix + [self.executable] + self.early_opts + opts - def start(self, stdin=None, wait_for_initialized=True): + def start(self, stdin=None, wait_for_initialized=True, stderr_redir=False): self.opts['bitcoin-rpcport'] = self.rpcproxy.rpcport - TailableProc.start(self, stdin, stdout_redir=False) + TailableProc.start(self, stdin, stdout_redir=False, stderr_redir=stderr_redir) if wait_for_initialized: self.wait_for_log("Server started with public key") logging.info("LightningD started") @@ -895,8 +901,8 @@ class LightningNode(object): info = self.rpc.getinfo() return 'warning_bitcoind_sync' not in info and 'warning_lightningd_sync' not in info - def start(self, wait_for_bitcoind_sync=True): - self.daemon.start() + def start(self, wait_for_bitcoind_sync=True, stderr_redir=False): + self.daemon.start(stderr_redir=stderr_redir) # Cache `getinfo`, we'll be using it a lot self.info = self.rpc.getinfo() # This shortcut is sufficient for our simple tests. diff --git a/tests/test_db.py b/tests/test_db.py index 1c4b7c35e..c21f5b4a8 100644 --- a/tests/test_db.py +++ b/tests/test_db.py @@ -425,7 +425,7 @@ def test_db_sanity_checks(bitcoind, node_factory): # Provide the --wallet option and start with wrong db l1.daemon.opts['wallet'] = "sqlite3://" + l2.db.path - l1.daemon.start(wait_for_initialized=False) + l1.daemon.start(wait_for_initialized=False, stderr_redir=True) l1.daemon.wait_for_log(r'\*\*BROKEN\*\* wallet: Wallet node_id does not match HSM') # Will have exited with non-zero status. assert l1.daemon.proc.wait(TIMEOUT) != 0 @@ -435,7 +435,7 @@ def test_db_sanity_checks(bitcoind, node_factory): l1.daemon.opts['wallet'] = "sqlite3://" + l1.db.path l1.daemon.opts['network'] = "bitcoin" - l1.daemon.start(wait_for_initialized=False) + l1.daemon.start(wait_for_initialized=False, stderr_redir=True) l1.daemon.wait_for_log(r'\*\*BROKEN\*\* wallet: Wallet blockchain hash does not match network blockchain hash') # Will have exited with non-zero status. assert l1.daemon.proc.wait(TIMEOUT) != 0 diff --git a/tests/test_gossip.py b/tests/test_gossip.py index adb72faad..4ce2f7967 100644 --- a/tests/test_gossip.py +++ b/tests/test_gossip.py @@ -241,8 +241,9 @@ def test_announce_and_connect_via_dns(node_factory, bitcoind): @unittest.skipIf(not EXPERIMENTAL_FEATURES, "BOLT7 DNS RFC #911") def test_only_announce_one_dns(node_factory, bitcoind): # and test that we can't announce more than one DNS address - l1 = node_factory.get_node(may_fail=True, expect_fail=True, + l1 = node_factory.get_node(expect_fail=True, start=False, options={'announce-addr': ['localhost.localdomain:12345', 'example.com:12345']}) + l1.daemon.start(wait_for_initialized=False, stderr_redir=True) wait_for(lambda: l1.daemon.is_in_stderr("Only one DNS can be announced")) diff --git a/tests/test_misc.py b/tests/test_misc.py index a0a834cce..83862b994 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -109,7 +109,7 @@ def test_bitcoin_failure(node_factory, bitcoind): # Ignore BROKEN log message about blocksonly mode. l2 = node_factory.get_node(start=False, expect_fail=True, allow_broken_log=True) - l2.daemon.start(wait_for_initialized=False) + l2.daemon.start(wait_for_initialized=False, stderr_redir=True) # Will exit with failure code. assert l2.daemon.wait() == 1 assert l2.daemon.is_in_stderr(r".*deactivating transaction relay is not" @@ -1212,7 +1212,7 @@ def test_rescan(node_factory, bitcoind): l1.daemon.opts['rescan'] = -500000 l1.stop() bitcoind.generate_block(4) - l1.daemon.start(wait_for_initialized=False) + l1.daemon.start(wait_for_initialized=False, stderr_redir=True) # Will exit with failure code. assert l1.daemon.wait() == 1 assert l1.daemon.is_in_stderr(r"bitcoind has gone backwards from 500000 to 105 blocks!") @@ -1243,7 +1243,7 @@ def test_bitcoind_goes_backwards(node_factory, bitcoind): bitcoind.start() # Will simply refuse to start. - l1.daemon.start(wait_for_initialized=False) + l1.daemon.start(wait_for_initialized=False, stderr_redir=True) # Will exit with failure code. assert l1.daemon.wait() == 1 assert l1.daemon.is_in_stderr('bitcoind has gone backwards') @@ -1252,7 +1252,7 @@ def test_bitcoind_goes_backwards(node_factory, bitcoind): l1.daemon.opts['rescan'] = 3 # Will simply refuse to start. - l1.daemon.start(wait_for_initialized=False) + l1.daemon.start(wait_for_initialized=False, stderr_redir=True) # Will exit with failure code. assert l1.daemon.wait() == 1 assert l1.daemon.is_in_stderr('bitcoind has gone backwards') diff --git a/tests/test_plugin.py b/tests/test_plugin.py index caf939246..87341b38e 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -109,7 +109,7 @@ def test_option_types(node_factory): }, may_fail=True, start=False) # the node should fail after start, and we get a stderr msg - n.daemon.start(wait_for_initialized=False) + n.daemon.start(wait_for_initialized=False, stderr_redir=True) assert n.daemon.wait() == 1 wait_for(lambda: n.daemon.is_in_stderr('bool_opt: ! does not parse as type bool')) @@ -122,7 +122,7 @@ def test_option_types(node_factory): }, may_fail=True, start=False) # the node should fail after start, and we get a stderr msg - n.daemon.start(wait_for_initialized=False) + n.daemon.start(wait_for_initialized=False, stderr_redir=True) assert n.daemon.wait() == 1 assert n.daemon.is_in_stderr('--int_opt: notok does not parse as type int') @@ -136,7 +136,7 @@ def test_option_types(node_factory): }, may_fail=True, start=False) # the node should fail after start, and we get a stderr msg - n.daemon.start(wait_for_initialized=False) + n.daemon.start(wait_for_initialized=False, stderr_redir=True) assert n.daemon.wait() == 1 assert n.daemon.is_in_stderr("--flag_opt: doesn't allow an argument") @@ -1522,7 +1522,7 @@ def test_libplugin(node_factory): l1.stop() l1.daemon.opts["name-deprecated"] = "test_opt" - l1.daemon.start(wait_for_initialized=False) + l1.daemon.start(wait_for_initialized=False, stderr_redir=True) # Will exit with failure code. assert l1.daemon.wait() == 1 assert l1.daemon.is_in_stderr(r"name-deprecated: deprecated option") @@ -1669,7 +1669,7 @@ def test_bitcoin_backend(node_factory, bitcoind): # We don't start if we haven't all the required methods registered. plugin = os.path.join(os.getcwd(), "tests/plugins/bitcoin/part1.py") l1.daemon.opts["plugin"] = plugin - l1.daemon.start(wait_for_initialized=False) + l1.daemon.start(wait_for_initialized=False, stderr_redir=True) l1.daemon.wait_for_log("Missing a Bitcoin plugin command") # Will exit with failure code. assert l1.daemon.wait() == 1 @@ -2094,7 +2094,7 @@ def test_important_plugin(node_factory): may_fail=True, expect_fail=True, allow_broken_log=True, start=False) - n.daemon.start(wait_for_initialized=False) + n.daemon.start(wait_for_initialized=False, stderr_redir=True) # Will exit with failure code. assert n.daemon.wait() == 1 assert n.daemon.is_in_stderr(r"Failed to register .*nonexistent: No such file or directory") diff --git a/tests/test_wallet.py b/tests/test_wallet.py index c50c39b40..ddee4c3d8 100644 --- a/tests/test_wallet.py +++ b/tests/test_wallet.py @@ -1038,7 +1038,7 @@ def test_hsm_secret_encryption(node_factory): # Test we cannot restore the same wallet with another password l1.daemon.opts.update({"encrypted-hsm": None}) - l1.daemon.start(stdin=slave_fd, wait_for_initialized=False) + l1.daemon.start(stdin=slave_fd, wait_for_initialized=False, stderr_redir=True) l1.daemon.wait_for_log(r'Enter hsm_secret password') write_all(master_fd, password[2:].encode("utf-8")) assert(l1.daemon.proc.wait(WAIT_TIMEOUT) == HSM_BAD_PASSWORD)