diff --git a/doc/HACKING.md b/doc/HACKING.md index 9d3237864679..6003dbd724ba 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 e1fdc12388bf..1bf3b690258b 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 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 678f03c48109..08dee44c98cf 100644 --- a/tests/test_cln_rs.py +++ b/tests/test_cln_rs.py @@ -5,19 +5,11 @@ 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):