Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: check for valgrind presence and set appropriate exit flags #17732

Open
wants to merge 1 commit into
base: master
from

Conversation

@brakmic
Copy link
Contributor

brakmic commented Dec 12, 2019

This PR fixes #17724.

It checks for valgrind's presence and version number.

This information will be used to decide whether the flag --exit-on-first-error should be active or not.

Versions >=3.14 will have it activated by default.

@brakmic brakmic force-pushed the brakmic:valgrind-detection branch from ffbffec to 61ebe59 Dec 12, 2019
@fanquake fanquake added the Tests label Dec 12, 2019
@brakmic brakmic force-pushed the brakmic:valgrind-detection branch from 61ebe59 to d9b5bd7 Dec 12, 2019
Copy link
Member

Sjors left a comment

Concept ACK

test/functional/test_framework/test_node.py Outdated Show resolved Hide resolved
test/functional/test_framework/test_node.py Outdated Show resolved Hide resolved
test/functional/test_framework/test_node.py Outdated Show resolved Hide resolved
Copy link
Member

jonatack left a comment

Concept/approach ACK. Could be helpful to describe how to test this in the PR description. A few nits below.

test/functional/test_framework/test_node.py Outdated Show resolved Hide resolved
test/functional/test_framework/test_node.py Outdated Show resolved Hide resolved
test/functional/test_framework/test_node.py Outdated Show resolved Hide resolved
test/functional/test_framework/test_node.py Outdated Show resolved Hide resolved
Copy link
Member

practicalswift left a comment

Concept ACK on the general idea of enabling --exit-on-first-error for Valgrind version 3.14 and above only :)

test/functional/test_framework/test_node.py Outdated Show resolved Hide resolved
test/functional/test_framework/test_node.py Outdated Show resolved Hide resolved
@brakmic brakmic force-pushed the brakmic:valgrind-detection branch 2 times, most recently from f595417 to 28eabc8 Dec 12, 2019
test/functional/test_framework/test_node.py Outdated Show resolved Hide resolved
test/functional/test_framework/test_node.py Outdated Show resolved Hide resolved
test/functional/test_framework/test_node.py Outdated Show resolved Hide resolved
@brakmic brakmic force-pushed the brakmic:valgrind-detection branch from 28eabc8 to 1c2f07c Dec 12, 2019
@brakmic brakmic force-pushed the brakmic:valgrind-detection branch 2 times, most recently from 9966afb to 1304359 Dec 12, 2019
@brakmic brakmic force-pushed the brakmic:valgrind-detection branch from 1304359 to 3ed9706 Dec 12, 2019
@brakmic brakmic force-pushed the brakmic:valgrind-detection branch from 3ed9706 to f10ca18 Dec 12, 2019
Copy link
Member

jonatack left a comment

Tested f10ca18 with valgrind 3.14, the code changes in my comments below (feel free to pick and choose), and this print, which show that the valgrind version is being parsed correctly and the exit flag set.

print("Running tests with valgrind {} and flag {}"
      .format(valgrind_version, self.valgrind_early_exit(valgrind_version)))
$ tool_wallet.py --valgrind
2019-12-12T20:09:32.902000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_7jnxbiit
Running tests with valgrind (3, 14) and flag --exit-on-first-error=yes
test/functional/test_framework/test_node.py Outdated Show resolved Hide resolved
test/functional/test_framework/test_node.py Outdated Show resolved Hide resolved
test/functional/test_framework/test_node.py Outdated Show resolved Hide resolved
@brakmic brakmic force-pushed the brakmic:valgrind-detection branch 3 times, most recently from 315ca94 to 34e7dc1 Dec 12, 2019
@brakmic brakmic force-pushed the brakmic:valgrind-detection branch from 34e7dc1 to 9a61f78 Dec 12, 2019
@jonatack

This comment has been minimized.

Copy link
Member

jonatack commented Dec 12, 2019

Partial tested ACK 9a61f78

After removing valgrind:

$ test/functional/tool_wallet.py  --valgrind
2019-12-12T21:23:04.053000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_0212zhle
2019-12-12T21:23:04.054000Z TestFramework (ERROR): Unexpected exception caught during testing
Traceback (most recent call last):
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 111, in main
    self.setup()
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 226, in setup
    self.setup_network()
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 315, in setup_network
    self.setup_nodes()
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 339, in setup_nodes
    self.add_nodes(self.num_nodes, extra_args)
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 403, in add_nodes
    use_valgrind=self.options.valgrind,
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_node.py", line 104, in __init__
    raise Exception('Valgrind not present')
Exception: Valgrind not present

After reinstalling valgrind 3.14 and adding this print statement to the bottom of the if use_valgrind block:

print("Running tests with valgrind {} and flag {}"
      .format(valgrind_version, self.valgrind_early_exit(valgrind_version)))
$ test/functional/tool_wallet.py  --valgrind
2019-12-12T21:24:04.216000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_w4dpyzgc
Running tests with valgrind (3, 14) and flag --exit-on-first-error=yes

Not tested: running tests with valgrind version < 3.14.

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Dec 16, 2019

On macOS I'm seeing the right error when valgrind is missing, but then it goes into an infinite recursion:

2019-12-16T12:47:33.219000Z TestFramework (ERROR): Unexpected exception caught during testing
Traceback (most recent call last):
  File "/Users/sjors/dev/bitcoin/test/functional/test_framework/test_framework.py", line 111, in main
    self.setup()
  File "/Users/sjors/dev/bitcoin/test/functional/test_framework/test_framework.py", line 226, in setup
    self.setup_network()
  File "test/functional/wallet_dump.py", line 98, in setup_network
    self.add_nodes(self.num_nodes, extra_args=self.extra_args)
  File "/Users/sjors/dev/bitcoin/test/functional/test_framework/test_framework.py", line 403, in add_nodes
    use_valgrind=self.options.valgrind,
  File "/Users/sjors/dev/bitcoin/test/functional/test_framework/test_node.py", line 104, in __init__
    raise Exception('Valgrind not present')
Exception: Valgrind not present
Exception ignored in: <bound method TestNode.__del__ of <test_framework.test_node.TestNode object at 0x103280be0>>
Traceback (most recent call last):
  File "/Users/sjors/dev/bitcoin/test/functional/test_framework/test_node.py", line 182, in __del__
    if self.process and self.cleanup_on_exit:
  File "/Users/sjors/dev/bitcoin/test/functional/test_framework/test_node.py", line 191, in __getattr__
    if self.use_cli:
  File "/Users/sjors/dev/bitcoin/test/functional/test_framework/test_node.py", line 191, in __getattr__
    if self.use_cli:
...
  File "/Users/sjors/dev/bitcoin/test/functional/test_framework/test_node.py", line 191, in __getattr__
    if self.use_cli:
RecursionError: maximum recursion depth exceeded
2019-12-16T12:47:33.302000Z TestFramework (INFO): Stopping nodes
@brakmic

This comment has been minimized.

Copy link
Contributor Author

brakmic commented Dec 16, 2019

On macOS I'm seeing the right error when valgrind is missing, but then it goes into an infinite recursion:

File "/Users/sjors/dev/bitcoin/test/functional/test_framework/test_node.py", line 191, in getattr
if self.use_cli:
RecursionError: maximum recursion depth exceeded
2019-12-16T12:47:33.302000Z TestFramework (INFO): Stopping nodes

The __ del __ method doesn't have a try-catch block. When I put one it prevents the recursion. Not sure if I should change unrelated code here. Maybe I should wait for input from people who coded it in the first place?

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Dec 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.