test: raise explicit error if any of the needed release binaries is missing #31462

pull theStack wants to merge 1 commits into bitcoin:master from theStack:202412-test-skip_if_release_binaries_missing changing 1 files +9 −0
  1. theStack commented at 6:53 pm on December 10, 2024: contributor

    If the releases directory exists, but still only a subset of the necessary previous release binaries are available, the test fails by throwing an exception (sometimes leading to follow-up exceptions like AssertionError: [node 0] Error: no RPC connection) and printing out a stack trace, which can be confusing and at a first glance suggests that the node crashed or some alike. Improve this by checking and printing out all of the missing release binaries and failing with an explicit error in this case. Also add an info on how to download previous releases binaries. Noticed while testing #30328.

    Can be tested by e.g.

    0$ rm -rf ./releases
1$ ./test/get_previous_releases.py -b
2$ rm -rf ./releases/v28.0/
3$ ./build/test/functional/wallet_migration.py

    master:

     0...
 12024-12-10T18:48:54.067000Z TestFramework (ERROR): Assertion failed
 2Traceback (most recent call last):
 3  File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 590, in start_nodes
 4    node.start(extra_args[i], *args, **kwargs)
 5  File "/home/thestack/bitcoin/test/functional/test_framework/test_node.py", line 257, in start
 6    self.process = subprocess.Popen(self.args + extra_args, env=subp_env, stdout=stdout, stderr=stderr, cwd=cwd, **kwargs)
 7  File "/usr/lib/python3.10/subprocess.py", line 971, in __init__
 8    self._execute_child(args, executable, preexec_fn, close_fds,
 9  File "/usr/lib/python3.10/subprocess.py", line 1863, in _execute_child
10    raise child_exception_type(errno_num, err_msg, err_filename)
11FileNotFoundError: [Errno 2] No such file or directory: '/home/thestack/bitcoin/releases/v28.0/bin/bitcoind'
12
13During handling of the above exception, another exception occurred:
14
15Traceback (most recent call last):
16  File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 131, in main
17    self.setup()
18  File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 315, in setup
19    self.setup_network()
20  File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 409, in setup_network
21    self.setup_nodes()
22  File "/home/thestack/bitcoin/./build/test/functional/wallet_migration.py", line 54, in setup_nodes
23    self.start_nodes()
24  File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 595, in start_nodes
25    self.stop_nodes()
26  File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 610, in stop_nodes
27    node.stop_node(wait=wait, wait_until_stopped=False)
28  File "/home/thestack/bitcoin/test/functional/test_framework/test_node.py", line 396, in stop_node
29    self.stop(wait=wait)
30  File "/home/thestack/bitcoin/test/functional/test_framework/test_node.py", line 215, in __getattr__
31    assert self.rpc_connected and self.rpc is not None, self._node_msg("Error: no RPC connection")
32AssertionError: [node 0] Error: no RPC connection
332024-12-10T18:48:54.118000Z TestFramework (INFO): Stopping nodes
34Traceback (most recent call last):
35  File "/home/thestack/bitcoin/./build/test/functional/wallet_migration.py", line 1097, in <module>
36    WalletMigrationTest(__file__).main()
37  File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 159, in main
38    exit_code = self.shutdown()
39  File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 331, in shutdown
40    self.stop_nodes()
41  File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 610, in stop_nodes
42    node.stop_node(wait=wait, wait_until_stopped=False)
43  File "/home/thestack/bitcoin/test/functional/test_framework/test_node.py", line 396, in stop_node
44    self.stop(wait=wait)
45  File "/home/thestack/bitcoin/test/functional/test_framework/test_node.py", line 215, in __getattr__
46    assert self.rpc_connected and self.rpc is not None, self._node_msg("Error: no RPC connection")
47AssertionError: [node 0] Error: no RPC connection
48[node 0] Cleaning up leftover process
49...

    PR:

     0...
 12025-01-01T20:26:27.999000Z TestFramework (INFO): PRNG seed is: 4570383538468804512
 22025-01-01T20:26:28.000000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_lz66_zcu
 32025-01-01T20:26:28.003000Z TestFramework (ERROR): Binary not found: /home/thestack/bitcoin/releases/v28.0/bin/bitcoind
 42025-01-01T20:26:28.003000Z TestFramework (ERROR): Binary not found: /home/thestack/bitcoin/releases/v28.0/bin/bitcoin-cli
 52025-01-01T20:26:28.003000Z TestFramework (INFO): Previous releases binaries can be downloaded via `test/get_previous_releases.py -b`.
 62025-01-01T20:26:28.003000Z TestFramework (ERROR): Assertion failed
 7Traceback (most recent call last):
 8  File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 131, in main
 9    self.setup()
10  File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 315, in setup
11    self.setup_network()
12  File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 409, in setup_network
13    self.setup_nodes()
14  File "/home/thestack/bitcoin/./build/test/functional/wallet_migration.py", line 50, in setup_nodes
15    self.add_nodes(self.num_nodes, versions=[
16  File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 537, in add_nodes
17    raise AssertionError("At least one release binary is missing")
18AssertionError: At least one release binary is missing
192025-01-01T20:26:28.061000Z TestFramework (INFO): Stopping nodes
20...
  2. DrahtBot commented at 6:53 pm on December 10, 2024: contributor

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

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31462.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK fjahr, kevkevinpal, maflcko, pablomartin4btc, achow101

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #31375 (multiprocess: Add bitcoin wrapper executable by ryanofsky)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  3. DrahtBot added the label Tests on Dec 10, 2024
  4. maflcko commented at 7:12 pm on December 10, 2024: member

    Not sure about the implementation. This will turn an error (a missing previous release bin) into a silently passing CI.

    FileNotFoundError: [Errno 2] No such file or directory: ‘/home/thestack/bitcoin/releases/v28.0/bin/bitcoind’

    Seems pretty self-explanatory and doesn’t need more code to write another error?

    My understanding from reading the code is that the Error: no RPC connection shouldn’t happen on stop. Since stop checks for self.running, and self.running = True is only set after Popen, which throws for you.

    If this isn’t the case, it would be better to fix that bug.

  5. kevkevinpal commented at 7:38 pm on December 10, 2024: contributor

    I agree that at first glance the log might be confusing but I agree with @maflcko that we should not be silently passing the ci

    I would prefer we raise an AssertionError instead, so we keep the same functionality of the error but a better output

    Output with AssertionError

     02024-12-10T19:35:01.177000Z TestFramework (INFO): PRNG seed is: 5297365276077502746
 12024-12-10T19:35:01.178000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_thepg93c
 22024-12-10T19:35:01.178000Z TestFramework (ERROR): Binary not found: /mnt/shared_drive/DEVDIR/bitcoin/releases/v28.0/bin/bitcoind
 32024-12-10T19:35:01.178000Z TestFramework (ERROR): Binary not found: /mnt/shared_drive/DEVDIR/bitcoin/releases/v28.0/bin/bitcoin-cli
 42024-12-10T19:35:01.178000Z TestFramework (ERROR): Assertion failed
 5Traceback (most recent call last):
 6  File "/mnt/shared_drive/DEVDIR/bitcoin/test/functional/test_framework/test_framework.py", line 131, in main
 7    self.setup()
 8  File "/mnt/shared_drive/DEVDIR/bitcoin/test/functional/test_framework/test_framework.py", line 315, in setup
 9    self.setup_network()
10  File "/mnt/shared_drive/DEVDIR/bitcoin/test/functional/test_framework/test_framework.py", line 409, in setup_network
11    self.setup_nodes()
12  File "/mnt/shared_drive/DEVDIR/bitcoin/./build/test/functional/wallet_migration.py", line 50, in setup_nodes
13    self.add_nodes(self.num_nodes, versions=[
14  File "/mnt/shared_drive/DEVDIR/bitcoin/test/functional/test_framework/test_framework.py", line 536, in add_nodes
15    raise AssertionError("At least one release binary is missing")
16AssertionError: At least one release binary is missing
172024-12-10T19:35:01.229000Z TestFramework (INFO): Stopping nodes
182024-12-10T19:35:01.229000Z TestFramework (WARNING): Not cleaning up dir /tmp/bitcoin_func_test_thepg93c
192024-12-10T19:35:01.229000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/bitcoin_func_test_thepg93c/test_framework.log
202024-12-10T19:35:01.229000Z TestFramework (ERROR):
212024-12-10T19:35:01.230000Z TestFramework (ERROR): Hint: Call /mnt/shared_drive/DEVDIR/bitcoin/test/functional/combine_logs.py '/tmp/bitcoin_func_test_thepg93c' to consolidate all logs
222024-12-10T19:35:01.230000Z TestFramework (ERROR):
232024-12-10T19:35:01.230000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
242024-12-10T19:35:01.230000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
252024-12-10T19:35:01.230000Z TestFramework (ERROR):

    nit: in the PR description to reproduce, I get No such file or directory ./test/get_previous_releases -b -> ./test/get_previous_releases.py -b

  6. fjahr commented at 3:54 pm on December 30, 2024: contributor
    I agree that the output isn’t ideal, I always forget what to run to get the missing binaries when I run into this. But I also think the test should still fail.
  7. theStack force-pushed on Jan 1, 2025
  8. theStack renamed this:
    test: skip test if any of the needed release binaries is missing
    test: raise explicit error if any of the needed release binaries is missing
    on Jan 1, 2025
  9. theStack commented at 8:33 pm on January 1, 2025: contributor
    Thanks for the reviews! I agree that skipping the test is a bad idea, switched to to an explicit error message instead as suggested, also added a log.info with instructions on how to obtain the binaries for previous releases in this case.
  10. theStack force-pushed on Jan 1, 2025
  11. DrahtBot added the label CI failed on Jan 1, 2025
  12. in test/functional/test_framework/test_framework.py:529 in e8795f4c11 outdated 
    525@@ -526,6 +526,15 @@ def get_bin_from_version(version, bin_name, bin_default):
526             binary = [get_bin_from_version(v, 'bitcoind', self.options.bitcoind) for v in versions]
527         if binary_cli is None:
528             binary_cli = [get_bin_from_version(v, 'bitcoin-cli', self.options.bitcoincli) for v in versions]
529+        # Skip test if any of the needed release binaries is missing
    fjahr commented at 9:24 pm on January 1, 2025: 
    0        # Fail test if any of the needed release binaries is missing
    theStack commented at 0:26 am on January 7, 2025:
    Fixed.
  13. DrahtBot removed the label CI failed on Jan 1, 2025
  14. in test/functional/test_framework/test_framework.py:537 in e8795f4c11 outdated 
    532+            if shutil.which(bin_path) is None:
533+                self.log.error(f"Binary not found: {bin_path}")
534+                bins_missing = True
535+        if bins_missing:
536+            self.log.info("Previous releases binaries can be downloaded via `test/get_previous_releases.py -b`.")
537+            raise AssertionError("At least one release binary is missing")
    fjahr commented at 10:50 am on January 2, 2025:

    nit: Could also been put into the assert message IMO

    0            raise AssertionError("At least one release binary is missing. Previous releases binaries can be downloaded via `test/get_previous_releases.py -b`.")
    furszy commented at 6:56 pm on January 6, 2025:
    +1. Something like this would be helpful. I’ve received a few messages sharing these logs without knowing what to do next.
    theStack commented at 0:27 am on January 7, 2025:
    Thanks, moved the download instructions directly into the assertion message as suggested.
    pablomartin4btc commented at 6:31 pm on January 8, 2025:

    +1. Something like this would be helpful. I’ve received a few messages sharing these logs without knowing what to do next.

    I was one of them :raising_hand:, I stepped upon this issue while testing #31451 and I found this answer from @achow101 in stack-overflow before checking /test/README.md.

    I’d put a reference to the README file in the error description instead of the .py and I’d clarify this issue there in the readme (if you agree with it but you don’t want to update the PR I can create a new one). Also, because I didn’t know about the -b [option], the script started to compile all versions specified in SHA256_SUMS (perhaps we should update the script and remove the old ones as they don’t compile… or even switch the defautl to -b and create another option in order to compile the missing version?).

    Last thing is that “At least one release” is not entirely correct, we need the last previous to the current, if you delete the v.28.0 from the releases folder (as specified in the description of the PR) and you still have v25.0 and/ or v24.0.1, you will get the same error (I think it’s how add_nodes() combined with get_bin_from_version() works from the test_framework works).

    theStack commented at 10:07 am on January 9, 2025:

    Will leave the PR as-is, but happy to review a follow-up PR which refines test/README.md about previous releases and/or adds improvements to the previous release download script itself.

    Last thing is that “At least one release” is not entirely correct, we need the last previous to the current, if you delete the v.28.0 from the releases folder (as specified in the #31462#issue-2730936530 of the PR) and you still have v25.0 and/ or v24.0.1, you will get the same error (I think it’s how add_nodes() combined with get_bin_from_version() works from the test_framework works).

    Note that the concrete list of needed releases is individual for each functional test (passed as versions parameter to the add_nodes call), wallet_migration.py needing v28.0 is just one example. Stating “At least one needed release binary is missing” might be slightly more accurate, will adapt if I have to retouch.

    pablomartin4btc commented at 2:16 pm on January 9, 2025:
    Oh, I see… yeah, ok… in the case of WalletMigrationTest I can see the required versions defined in the setup. Thank you!
  15. fjahr commented at 11:23 am on January 2, 2025: contributor

    tACK e8795f4c11622c26cf303b759ba717351a8fba45

    Would be good to fix the comment but it’s not a big deal.

    Here is the output I see, I think it would great if the backtrace could be suppressed in this case because it’s mostly noise distracting from the helpful logs. But it might be too much hassle, so not a must.

     02025-01-01T21:24:42.885000Z TestFramework (INFO): Initializing test directory /var/folders/9z/n7rz_6cj3bq__11k5kcrsvvm0000gn/T/bitcoin_func_test_n24yl9c0
 12025-01-01T21:24:42.886000Z TestFramework (ERROR): Binary not found: /Users/XXX/projects/clones/bitcoin/releases/v28.0/bin/bitcoind
 22025-01-01T21:24:42.886000Z TestFramework (ERROR): Binary not found: /Users/XX/projects/clones/bitcoin/releases/v28.0/bin/bitcoin-cli
 32025-01-01T21:24:42.886000Z TestFramework (INFO): Previous releases binaries can be downloaded via `test/get_previous_releases.py -b`.
 42025-01-01T21:24:42.886000Z TestFramework (ERROR): Assertion failed
 5Traceback (most recent call last):
 6  File "/Users/XXX/projects/clones/bitcoin/test/functional/test_framework/test_framework.py", line 131, in main
 7    self.setup()
 8    ~~~~~~~~~~^^
 9  File "/Users/XXX/projects/clones/bitcoin/test/functional/test_framework/test_framework.py", line 315, in setup
10    self.setup_network()
11    ~~~~~~~~~~~~~~~~~~^^
12  File "/Users/XXX/projects/clones/bitcoin/test/functional/test_framework/test_framework.py", line 409, in setup_network
13    self.setup_nodes()
14    ~~~~~~~~~~~~~~~~^^
15  File "/Users/XXX/projects/clones/bitcoin/build/test/functional/wallet_migration.py", line 50, in setup_nodes
16    self.add_nodes(self.num_nodes, versions=[
17    ~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^
18        None,
19        ^^^^^
20        280000,
21        ^^^^^^^
22    ])
23    ^^
24  File "/Users/XXX/projects/clones/bitcoin/test/functional/test_framework/test_framework.py", line 537, in add_nodes
25    raise AssertionError("At least one release binary is missing")
26AssertionError: At least one release binary is missing
272025-01-01T21:24:42.942000Z TestFramework (INFO): Stopping nodes
  16. test: raise explicit error if any of the needed release binaries is missing 
    If the `releases` directory exists, but still only a subset of the
necessary previous release binaries are available, the test fails by
throwing an exception (sometimes leading to follow-up exceptions like
"AssertionError: [node 0] Error: no RPC connection") and printing out
a stack trace, which can be confusing and at a first glance suggests
that the node crashed or some alike.
Improve this by checking and printing out *all* of the missing release
binaries and failing with an explicit error in this case. Also add an
info on how to download previous releases binaries.
Noticed while testing #30328.

Can be tested by e.g.

$ ./test/get_previous_releases.py -b
$ rm -rf ./releases/v28.0/
$ ./build/test/functional/wallet_migration.py
    1ea7e45a1f
  17. theStack force-pushed on Jan 7, 2025
  18. fjahr commented at 8:53 pm on January 7, 2025: contributor
    re-ACK 1ea7e45a1f445d32a2b690d52befb2e63418653b
  19. kevkevinpal commented at 9:22 pm on January 7, 2025: contributor
    ACK 1ea7e45
  20. maflcko commented at 10:40 am on January 8, 2025: member

    I’ve created #31620 to reduce the long, redundant and confusing error message observed originally a bit.

    But the two changes are unrelated otherwise.

    lgtm ACK 1ea7e45a1f445d32a2b690d52befb2e63418653b

  21. pablomartin4btc commented at 6:31 pm on January 8, 2025: member

    Concept ACK.

    Left a few suggestions but other than that I’m happy with this step forward.

  22. pablomartin4btc commented at 2:17 pm on January 9, 2025: member
    tACK 1ea7e45a1f445d32a2b690d52befb2e63418653b
  23. achow101 commented at 11:21 pm on January 9, 2025: member
    ACK 1ea7e45a1f445d32a2b690d52befb2e63418653b
  24. achow101 merged this on Jan 9, 2025
  25. achow101 closed this on Jan 9, 2025
  26. theStack deleted the branch on Jan 10, 2025


theStack DrahtBot maflcko kevkevinpal fjahr furszy pablomartin4btc achow101

Labels
Tests

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-01-21 06:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me