test: exempt previous release binaries from valgrind

Sjors commented at 2:55 pm on March 8, 2023: member

Some, but not all, backward compatibility tests fail for me and it seems useless to run old release binaries under valgrind anyway.

Can be tested by running test/functional/feature_txindex_compatibility.py --valgrind --timeout-factor=10 with and without this PR.

— The previous version of this PR disabled these test entirely under valgrind. The current version does run the test, but starts the old binaries without valgrind.

DrahtBot commented at 2:55 pm on March 8, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko
Approach NACK	luke-jr

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

Sjors commented at 2:55 pm on March 8, 2023: member

Example failure:

 0$ test/functional/feature_txindex_compatibility.py --valgrind --timeout-factor=4
 12023-03-08T14:43:48.054000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_c2c_8427
 22023-03-08T14:43:49.852000Z TestFramework (ERROR): Assertion failed
 3Traceback (most recent call last):
 4  File "/home/sjors/dev/bitcoin/test/functional/test_framework/test_framework.py", line 559, in start_nodes
 5    node.wait_for_rpc_connection()
 6  File "/home/sjors/dev/bitcoin/test/functional/test_framework/test_node.py", line 232, in wait_for_rpc_connection
 7    'bitcoind exited with status {} during initialization'.format(self.process.returncode)))
 8test_framework.test_node.FailedToStartError: [node 0] bitcoind exited with status 1 during initialization
 9
10During handling of the above exception, another exception occurred:
11
12Traceback (most recent call last):
13  File "/home/sjors/dev/bitcoin/test/functional/test_framework/test_framework.py", line 133, in main
14    self.setup()
15  File "/home/sjors/dev/bitcoin/test/functional/test_framework/test_framework.py", line 297, in setup
16    self.setup_network()
17  File "test/functional/feature_txindex_compatibility.py", line 39, in setup_network
18    self.start_nodes()
19  File "/home/sjors/dev/bitcoin/test/functional/test_framework/test_framework.py", line 562, in start_nodes
20    self.stop_nodes()
21  File "/home/sjors/dev/bitcoin/test/functional/test_framework/test_framework.py", line 577, in stop_nodes
22    node.stop_node(wait=wait, wait_until_stopped=False)
23  File "/home/sjors/dev/bitcoin/test/functional/test_framework/test_node.py", line 344, in stop_node
24    self.stop()
25  File "/home/sjors/dev/bitcoin/test/functional/test_framework/test_node.py", line 190, in __getattr__
26    assert self.rpc_connected and self.rpc is not None, self._node_msg("Error: no RPC connection")
27AssertionError: [node 0] Error: no RPC connection
282023-03-08T14:43:49.903000Z TestFramework (INFO): Stopping nodes
29Traceback (most recent call last):
30  File "test/functional/feature_txindex_compatibility.py", line 91, in <module>
31    TxindexCompatibilityTest().main()
32  File "/home/sjors/dev/bitcoin/test/functional/test_framework/test_framework.py", line 157, in main
33    exit_code = self.shutdown()
34  File "/home/sjors/dev/bitcoin/test/functional/test_framework/test_framework.py", line 313, in shutdown
35    self.stop_nodes()
36  File "/home/sjors/dev/bitcoin/test/functional/test_framework/test_framework.py", line 577, in stop_nodes
37    node.stop_node(wait=wait, wait_until_stopped=False)
38  File "/home/sjors/dev/bitcoin/test/functional/test_framework/test_node.py", line 344, in stop_node
39    self.stop()
40  File "/home/sjors/dev/bitcoin/test/functional/test_framework/test_node.py", line 190, in __getattr__
41    assert self.rpc_connected and self.rpc is not None, self._node_msg("Error: no RPC connection")
42AssertionError: [node 0] Error: no RPC connection
43[node 2] Cleaning up leftover process
44[node 1] Cleaning up leftover process
45[node 0] Cleaning up leftover process

DrahtBot added the label Tests on Mar 8, 2023

maflcko commented at 3:01 pm on March 8, 2023: member

What about test/functional/mempool_compatibility.py etc?

Sjors commented at 3:02 pm on March 8, 2023: member

For some reason that didn’t fail. So an alternative could be to fix the ones that break for me, but not sure if that’s worth the effort.

Sjors renamed this:
~~test: skip backward compatibility tests under valgrind~~
test: skip some backward compatibility tests under valgrind
on Mar 8, 2023

maflcko commented at 4:56 pm on March 8, 2023: member

It might be more useful to disable them all here for the compat test only and instead run the full test suite on the guix binaries on release tags.

fanquake commented at 5:01 pm on March 8, 2023: member

This seems arbitrary, and the can't run this test under valgrind message is not useful. I assume the first questions someone will have are going to be “Why can’t I run this test under Valgrind?”, “Does it need fixing then?” etc.

maflcko commented at 5:14 pm on March 8, 2023: member

I presume the reason is that the valgrind supp file is not valid for previous releases. Actually, it might not even be valid for guix-compiled binaries on master?

Sjors force-pushed on Mar 8, 2023

Sjors commented at 5:31 pm on March 8, 2023: member

I disabled all tests that use previous releases under valgrind. Also tried a different error message.

“Does it need fixing then?”

I added a code comment and some more context to the commit to help answer that, if anyone does want to run these under valgrind.

in test/functional/feature_txindex_compatibility.py:28 in 5a0f71a598 outdated

24@@ -25,6 +25,8 @@ def set_test_params(self):
25 
26     def skip_test_if_missing_module(self):
27         self.skip_if_no_previous_releases()
28+        # Not all release binaries work with valgrind.

maflcko commented at 5:32 pm on March 8, 2023:

nit: Might be a smaller diff, and more clear, if this was put into the skip_if_no_previous_releases function?

Sjors commented at 5:33 pm on March 8, 2023:

Thought about that, but there may be other (future) tests we want to skip under valgrind (e.g. things that use a brittle dependency). Conversely, since some tests do work, maybe someone wants to enable one later.

fanquake commented at 5:33 pm on March 8, 2023:

Yea. If they are all going to be disabled in that case any way, no point duplicating all this logic and commentary.

fanquake commented at 5:36 pm on March 8, 2023:

(future) tests we want to skip under valgrind

Let’s not worry about unwritten tests, that we apparently might not want to test properly for some arbitrary reason.

Sjors commented at 5:39 pm on March 8, 2023:

I was also thinking existing tests, e.g. I haven’t been able to run valgrind with BDB configured (didn’t try very hard).

fanquake commented at 5:44 pm on March 8, 2023:

I was also thinking existing tests, e.g. I haven’t been able to run valgrind with BDB configured (didn’t try very hard).

It definitely works. The native valgrind CI job is already configured to run with BDB.

maflcko commented at 10:01 am on March 9, 2023:

Conversely, since some tests do work, maybe someone wants to enable one later.

If you want to make it more fine grained, it could make sense to move the check to the test_node and condition it on the version number?

BDB

Good point. I also can’t run --valgrind with BDB on my system. Which raises the question how many settings we want --valgrind to be supported under.

fanquake commented at 10:06 am on March 9, 2023:

Which raises the question how many settings we want –valgrind to be supported under.

I thnk currently the only realistic place we can fully support Valgrind, is within the CI system, where things like BDB are working, and we can more easily control settings and suppressions. Outside of that, things may work fine, or may not at all, depending on the system where it is being run.

Sjors renamed this:
~~test: skip some backward compatibility tests under valgrind~~
test: skip all backward compatibility tests under valgrind
on Mar 8, 2023

luke-jr changes_requested

luke-jr commented at 8:57 pm on June 22, 2023: member

Approach NACK: Would prefer to run all the tests, but only execute the older binaries without Valgrind (ie, still run the current version with Valgrind)

Sjors commented at 7:53 am on June 27, 2023: member

Running the old binaries without valgrind makes sense. I’ll see if I can figure out how.

Sjors force-pushed on Aug 21, 2023

Sjors renamed this:
~~test: skip all backward compatibility tests under valgrind~~
test: exempt previous release binaries from valgrind
on Aug 21, 2023

maflcko commented at 10:29 am on August 21, 2023: member

lgtm ACK 3eb9bd7ccae6366eb548fa79cbdf78816354dc60

Sjors commented at 10:29 am on August 21, 2023: member

Changed this PR so that it now runs the old binaries without valgrind, rather than skipping the backward compatibility test entirely.

DrahtBot added the label CI failed on Aug 28, 2023

maflcko requested review from luke-jr on Sep 3, 2023

DrahtBot removed the label CI failed on Sep 3, 2023

DrahtBot added the label Needs rebase on Oct 11, 2023

test: don't run old binaries under valgrind

This is unnecessary and caused test failures. The backward
compatibility tests are meant to find regressions in the
current codebase, not to detect bugs in older releases.

850670e3d6

Sjors force-pushed on Oct 12, 2023

Sjors commented at 7:28 am on October 12, 2023: member

Rebased.

maflcko commented at 8:08 am on October 12, 2023: member

lgtm ACK 850670e3d63ed7d04b417a43cb8ab06292aa2c23

DrahtBot removed the label Needs rebase on Oct 12, 2023

fanquake merged this on Oct 12, 2023

fanquake closed this on Oct 12, 2023

Sjors deleted the branch on Oct 12, 2023

Frank-GER referenced this in commit 2a3a6d896b on Oct 13, 2023

bitcoin locked this on Oct 11, 2024

test: exempt previous release binaries from valgrind #27228

Code Coverage

Reviews

Conflicts