From 4aba119733358662b11c7ac435ecfb59f40d32e3 Mon Sep 17 00:00:00 2001
From: Christian Decker <decker.christian@gmail.com>
Date: Wed, 9 Feb 2022 17:10:09 +0100
Subject: [PATCH] pytest: Use valgrind target suppressions instead of skipping
 tests

Having a list of very targeted suppressions allows us to still run the
majority of tests with valgrind checking, and not fail when Rust does
some trickery. This is for example the case with `std::sync::Once`
which uses `num_procs` calling out to the cgroups subsystem, sometimes
with a null path.

Suggested-by: Rusty Russell <@rustyrussell>
---
 doc/HACKING.md                  | 31 +++++++++++++++++++++++++++++++
 tests/fixtures.py               | 15 ++++++++++++++-
 tests/test_cln_rs.py            | 18 +++++-------------
 tests/valgrind-suppressions.txt | 14 ++++++++++++++
 4 files changed, 64 insertions(+), 14 deletions(-)
 create mode 100644 tests/valgrind-suppressions.txt

diff --git a/doc/HACKING.md b/doc/HACKING.md
index 9d3237864..6003dbd72 100644
--- a/doc/HACKING.md
+++ b/doc/HACKING.md
@@ -242,6 +242,37 @@ TEST_DB_PROVIDER=[sqlite3|postgres] - Selects the database to use when running
 EXPERIMENTAL_DUAL_FUND=[0|1]	    - Enable dual-funding tests.
 ```
 
+#### Troubleshooting
+
+##### Valgrind complains about code we don't control
+
+Sometimes `valgrind` will complain about code we do not control
+ourselves, either because it's in a library we use or it's a false
+positive. There are generally three ways to address these issues
+(in descending order of preference):
+
+ 1. Add a suppression for the one specific call that is causing the
+    issue. Upon finding an issue `valgrind` is instructed in the
+    testing framework to print filters that'd match the issue. These
+    can be added to the suppressions file under
+    `tests/valgrind-suppressions.txt` in order to explicitly skip
+    reporting these in future. This is preferred over the other
+    solutions since it only disables reporting selectively for things
+    that were manually checked. See the [valgrind docs][vg-supp] for
+    details.
+ 2. Add the process that `valgrind` is complaining about to the
+    `--trace-children-skip` argument in `pyln-testing`. This is used
+    in cases of full binaries not being under our control, such as the
+    `python3` interpreter used in tests that run plugins. Do not use
+    this for binaries that are compiled from our code, as it tends to
+    mask real issues.
+ 3. Mark the test as skipped if running under `valgrind`. It's mostly
+    used to skip tests that otherwise would take considerably too long
+    to test on CI. We discourage this for suppressions, since it is a
+    very blunt tool.
+
+[vg-supp]: https://valgrind.org/docs/manual/manual-core.html#manual-core.suppress
+
 Making BOLT Modifications
 -------------------------
 
diff --git a/tests/fixtures.py b/tests/fixtures.py
index e1fdc1238..84258367d 100644
--- a/tests/fixtures.py
+++ b/tests/fixtures.py
@@ -1,7 +1,8 @@
-from utils import DEVELOPER, TEST_NETWORK  # noqa: F401,F403
+from utils import DEVELOPER, TEST_NETWORK, VALGRIND  # noqa: F401,F403
 from pyln.testing.fixtures import directory, test_base_dir, test_name, chainparams, node_factory, bitcoind, teardown_checks, throttler, db_provider, executor, setup_logging, jsonschemas  # noqa: F401,F403
 from pyln.testing import utils
 from utils import COMPAT
+from pathlib import Path
 
 import os
 import pytest
@@ -17,6 +18,18 @@ class LightningNode(utils.LightningNode):
     def __init__(self, *args, **kwargs):
         utils.LightningNode.__init__(self, *args, **kwargs)
 
+        # We have some valgrind suppressions in the `tests/`
+        # directory, so we can add these to the valgrind configuration
+        # (not generally true when running pyln-testing, hence why
+        # it's being done in this specialization, and not in the
+        # library).
+        if self.daemon.cmd_line[0] == 'valgrind':
+            suppressions_path = Path(__file__).parent / "valgrind-suppressions.txt"
+            self.daemon.cmd_prefix += [
+                f"--suppressions={suppressions_path}",
+                "--gen-suppressions=all"
+            ]
+
         # If we opted into checking the DB statements we will attach the dblog
         # plugin before starting the node
         check_dblog = os.environ.get("TEST_CHECK_DBSTMTS", None) == "1"
diff --git a/tests/test_cln_rs.py b/tests/test_cln_rs.py
index 678f03c48..08dee44c9 100644
--- a/tests/test_cln_rs.py
+++ b/tests/test_cln_rs.py
@@ -5,19 +5,11 @@ import subprocess
 import pytest
 
 
-# Skip the entire module if we don't have Rust. The same is true for
-# VALGRIND, since it sometimes causes false positives in
-# `std::sync::Once`
-pytestmark = [
-    pytest.mark.skipif(
-        env('RUST') != '1',
-        reason='RUST is not enabled skipping rust-dependent tests'
-    ),
-    pytest.mark.skipif(
-        env('VALGRIND') == '1',
-        reason='VALGRIND is enabled skipping rust-dependent tests, as they may report false positives.'
-    ),
-]
+# Skip the entire module if we don't have Rust.
+pytestmark = pytest.mark.skipif(
+    env('RUST') != '1',
+    reason='RUST is not enabled skipping rust-dependent tests'
+)
 
 
 def test_rpc_client(node_factory):
diff --git a/tests/valgrind-suppressions.txt b/tests/valgrind-suppressions.txt
new file mode 100644
index 000000000..c0053e40e
--- /dev/null
+++ b/tests/valgrind-suppressions.txt
@@ -0,0 +1,14 @@
+{
+   rust__std_sync_once__try_statx_01
+   Memcheck:Param
+   statx(buf)
+   fun:statx
+   ...
+}
+{
+   rust__std_sync_once__try_statx_02
+   Memcheck:Param
+   statx(file_name)
+   fun:statx
+   ...
+}