test: exempt previous release binaries from valgrind #27228

Sjors · 2023-03-08T14:55:21Z

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 · 2023-03-08T14:55:32Z

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 · 2023-03-08T14:55:32Z

Example failure:

$ test/functional/feature_txindex_compatibility.py --valgrind --timeout-factor=4
2023-03-08T14:43:48.054000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_c2c_8427
2023-03-08T14:43:49.852000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/home/sjors/dev/bitcoin/test/functional/test_framework/test_framework.py", line 559, in start_nodes
    node.wait_for_rpc_connection()
  File "/home/sjors/dev/bitcoin/test/functional/test_framework/test_node.py", line 232, in wait_for_rpc_connection
    'bitcoind exited with status {} during initialization'.format(self.process.returncode)))
test_framework.test_node.FailedToStartError: [node 0] bitcoind exited with status 1 during initialization

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/sjors/dev/bitcoin/test/functional/test_framework/test_framework.py", line 133, in main
    self.setup()
  File "/home/sjors/dev/bitcoin/test/functional/test_framework/test_framework.py", line 297, in setup
    self.setup_network()
  File "test/functional/feature_txindex_compatibility.py", line 39, in setup_network
    self.start_nodes()
  File "/home/sjors/dev/bitcoin/test/functional/test_framework/test_framework.py", line 562, in start_nodes
    self.stop_nodes()
  File "/home/sjors/dev/bitcoin/test/functional/test_framework/test_framework.py", line 577, in stop_nodes
    node.stop_node(wait=wait, wait_until_stopped=False)
  File "/home/sjors/dev/bitcoin/test/functional/test_framework/test_node.py", line 344, in stop_node
    self.stop()
  File "/home/sjors/dev/bitcoin/test/functional/test_framework/test_node.py", line 190, in __getattr__
    assert self.rpc_connected and self.rpc is not None, self._node_msg("Error: no RPC connection")
AssertionError: [node 0] Error: no RPC connection
2023-03-08T14:43:49.903000Z TestFramework (INFO): Stopping nodes
Traceback (most recent call last):
  File "test/functional/feature_txindex_compatibility.py", line 91, in <module>
    TxindexCompatibilityTest().main()
  File "/home/sjors/dev/bitcoin/test/functional/test_framework/test_framework.py", line 157, in main
    exit_code = self.shutdown()
  File "/home/sjors/dev/bitcoin/test/functional/test_framework/test_framework.py", line 313, in shutdown
    self.stop_nodes()
  File "/home/sjors/dev/bitcoin/test/functional/test_framework/test_framework.py", line 577, in stop_nodes
    node.stop_node(wait=wait, wait_until_stopped=False)
  File "/home/sjors/dev/bitcoin/test/functional/test_framework/test_node.py", line 344, in stop_node
    self.stop()
  File "/home/sjors/dev/bitcoin/test/functional/test_framework/test_node.py", line 190, in __getattr__
    assert self.rpc_connected and self.rpc is not None, self._node_msg("Error: no RPC connection")
AssertionError: [node 0] Error: no RPC connection
[node 2] Cleaning up leftover process
[node 1] Cleaning up leftover process
[node 0] Cleaning up leftover process

maflcko · 2023-03-08T15:01:21Z

What about test/functional/mempool_compatibility.py etc?

Sjors · 2023-03-08T15:02:16Z

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.

maflcko · 2023-03-08T16:56:56Z

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 · 2023-03-08T17:01:57Z

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 · 2023-03-08T17:14:51Z

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 · 2023-03-08T17:31:17Z

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.

maflcko · 2023-03-08T17:32:37Z

test/functional/feature_txindex_compatibility.py

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

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.

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

(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.

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

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.

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.

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.

luke-jr

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 · 2023-06-27T07:53:46Z

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

maflcko · 2023-08-21T10:29:10Z

lgtm ACK 3eb9bd7ccae6366eb548fa79cbdf78816354dc60

Sjors · 2023-08-21T10:29:12Z

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

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.

Sjors · 2023-10-12T07:28:10Z

Rebased.

maflcko · 2023-10-12T08:08:19Z

lgtm ACK 850670e

…rind 850670e test: don't run old binaries under valgrind (Sjors Provoost) Pull request description: 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. ACKs for top commit: maflcko: lgtm ACK 850670e Tree-SHA512: ebdf461083f1292528e6619963b910f486b60b4f6b183f0aea2c8bfcafa98caeb204d138700cd288450643bcec5e49e12b89f2f7537fccdf495a2a33acd9cea0

DrahtBot added the Tests label Mar 8, 2023

Sjors mentioned this pull request Mar 8, 2023

test: Default timeout factor to 4 under --valgrind #27221

Merged

Sjors changed the title ~~test: skip backward compatibility tests under valgrind~~ test: skip some backward compatibility tests under valgrind Mar 8, 2023

Sjors force-pushed the 2023/03/valgrind_prev_release branch from d1f6e7e to 5a0f71a Compare March 8, 2023 17:30

maflcko reviewed Mar 8, 2023

View reviewed changes

Sjors changed the title ~~test: skip some backward compatibility tests under valgrind~~ test: skip all backward compatibility tests under valgrind Mar 8, 2023

luke-jr suggested changes Jun 22, 2023

View reviewed changes

Sjors force-pushed the 2023/03/valgrind_prev_release branch from 5a0f71a to 3eb9bd7 Compare August 21, 2023 10:24

Sjors changed the title ~~test: skip all backward compatibility tests under valgrind~~ test: exempt previous release binaries from valgrind Aug 21, 2023

DrahtBot added the CI failed label Aug 28, 2023

DrahtBot mentioned this pull request Sep 2, 2023

test: Use pathlib over os path #28392

Merged

maflcko requested a review from luke-jr September 3, 2023 10:13

DrahtBot removed the CI failed label Sep 3, 2023

DrahtBot added the Needs rebase label Oct 11, 2023

test: don't run old binaries under valgrind

850670e

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.

Sjors force-pushed the 2023/03/valgrind_prev_release branch from 3eb9bd7 to 850670e Compare October 12, 2023 07:26

DrahtBot removed the Needs rebase label Oct 12, 2023

fanquake merged commit a927d5c into bitcoin:master Oct 12, 2023

Sjors deleted the 2023/03/valgrind_prev_release branch October 12, 2023 09:58

bitcoin locked and limited conversation to collaborators Oct 11, 2024

test: exempt previous release binaries from valgrind #27228

test: exempt previous release binaries from valgrind #27228

Uh oh!

Conversation

Sjors commented Mar 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Mar 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

Sjors commented Mar 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Mar 8, 2023

Uh oh!

Sjors commented Mar 8, 2023

Uh oh!

maflcko commented Mar 8, 2023

Uh oh!

fanquake commented Mar 8, 2023

Uh oh!

maflcko commented Mar 8, 2023

Uh oh!

Sjors commented Mar 8, 2023

Uh oh!

maflcko Mar 8, 2023

Choose a reason for hiding this comment

Uh oh!

Sjors Mar 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fanquake Mar 8, 2023

Choose a reason for hiding this comment

Uh oh!

fanquake Mar 8, 2023

Choose a reason for hiding this comment

Uh oh!

Sjors Mar 8, 2023

Choose a reason for hiding this comment

Uh oh!

fanquake Mar 8, 2023

Choose a reason for hiding this comment

Uh oh!

maflcko Mar 9, 2023

Choose a reason for hiding this comment

Uh oh!

fanquake Mar 9, 2023

Choose a reason for hiding this comment

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors commented Jun 27, 2023

Uh oh!

maflcko commented Aug 21, 2023

Uh oh!

Sjors commented Aug 21, 2023

Uh oh!

Sjors commented Oct 12, 2023

Uh oh!

maflcko commented Oct 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Sjors commented Mar 8, 2023 •

edited

Loading

DrahtBot commented Mar 8, 2023 •

edited

Loading

Sjors commented Mar 8, 2023 •

edited

Loading

Sjors Mar 8, 2023 •

edited

Loading