test: fix TestShell initialization and reset() #31415

BrandonOdiwuor commented at 5:03 PM on December 3, 2024: contributor

Fixes TestShell initialization issues caused by resolving symlinks and looking for config.ini in the source path instead of the build path after migration to CMake (see #31131 (comment))

https://github.com/bitcoin/bitcoin/blob/ebe4cac38bf6c510b00b8e080acab079c54016d6/test/functional/test_framework/test_shell.py#L77 also fixes #31131

How to test:

$ python3
>>> import sys
>>> sys.path.insert(0, "./path/to/bitcoin/build/test/functional")
>>> from test_framework.test_shell import TestShell
>>> TestShell().setup(num_nodes=2, setup_clean_chain=True)
>>> TestShell().shutdown()
>>> TestShell.reset()

pinheadmz approved

pinheadmz commented at 4:41 PM on December 16, 2024: member

ACK 20d46f30232ddeb92b6158aa1240be065fb2f8c4

Confirmed the issue and the fix.

Tested with:

#!/usr/bin/env python3

# USAGE: shell.py /path/to/bitcoin/repo

from pathlib import Path
import sys
REPO = Path(sys.argv.pop())
sys.path.insert(0, f"{REPO / 'build' / 'test' / 'functional'}")
from test_framework.test_shell import TestShell

TestShell().setup(num_nodes=1, setup_clean_chain=True)
TestShell().shutdown()

<details><summary>Show Signature</summary>

pinheadmz's public key is on keybase

</details>

DrahtBot commented at 4:41 PM on December 16, 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/31415.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	pinheadmz, theStack, pablomartin4btc

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

pinheadmz commented at 4:46 PM on December 16, 2024: member

You also might want to include a note in the docs like I did in #31510 since there are two test_framework directories after building now

BrandonOdiwuor force-pushed on Dec 18, 2024

BrandonOdiwuor requested review from pinheadmz on Dec 18, 2024

BrandonOdiwuor commented at 4:59 AM on December 18, 2024: contributor

@pinheadmz updated the test-shell doc

pinheadmz approved

pinheadmz commented at 3:47 PM on December 18, 2024: member

re-ACK 318b2a2f90dfd8ae20beca58e55e6d240a7f3d27

Docs updated since last ack, good improvement thank you!

<details><summary>Show Signature</summary>

pinheadmz's public key is on keybase

</details>

in test/functional/test-shell.md:36 in 318b2a2f90 outdated

  34 | +must be used.
  35 |  
  36 |  ```
  37 |  >>> import sys
  38 | ->>> sys.path.insert(0, "/path/to/bitcoin/test/functional")
  39 | +>>> sys.path.insert(0, "/path/to/bitcoin/build/test/functional")

l0rinc commented at 3:21 PM on December 20, 2024:

there are many other instances of bitcoin/test in the doc, could you please adjust the rest as well?

BrandonOdiwuor commented at 12:27 PM on February 20, 2025:

fixed the remaining instances

in test/functional/test_framework/test_shell.py:12 in 318b2a2f90 outdated

   6 | @@ -7,6 +7,14 @@
   7 |  from test_framework.test_framework import BitcoinTestFramework
   8 |  
   9 |  
  10 | +# BitcoinTestFramework instances are supposed to be constructed with the path
  11 | +# of the calling test in order to find shared data like configuration and the
  12 | +# cache. Since TestShell is meant for interactive use, there is no concrete

l0rinc commented at 3:29 PM on December 20, 2024:

Now that it's moved out of TestShell this (slightly awkward) comment should likely be updated.

BrandonOdiwuor commented at 12:30 PM on February 20, 2025:

reverted the extraction of dummy_testshell_file to avoid global variables

in test/functional/test_framework/test_shell.py:16 in 318b2a2f90 outdated

  11 | +# of the calling test in order to find shared data like configuration and the
  12 | +# cache. Since TestShell is meant for interactive use, there is no concrete
  13 | +# test; passing a dummy name is fine though, as only the containing directory
  14 | +# is relevant for successful initialization.
  15 | +tests_directory = pathlib.Path(__file__).absolute().parent.parent
  16 | +dummy_testshell_file = tests_directory / "testshell_dummy.py"

l0rinc commented at 3:30 PM on December 20, 2024:

Q: I'm not sure I understand the reason for extraction, can you please clarify that?

Nit: We're adding another global here which pollutes the namespace, can we rather inline it?

dummy_testshell_file = pathlib.Path(__file__).absolute().parent.parent / "testshell_dummy.py"

BrandonOdiwuor commented at 12:32 PM on February 20, 2025:

The reason for the extraction was to share the variable with the reset function, I have however reverted the extraction to avoid using global variables

in test/functional/test_framework/test_shell.py:71 in 318b2a2f90 outdated

  74 |  
  75 |      instance = None
  76 |  
  77 |      def __new__(cls):
  78 |          # This implementation enforces singleton pattern, and will return the
  79 |          # previously initialized instance if available

l0rinc commented at 3:35 PM on December 20, 2024:

useless comment, the code already makes this obvious

BrandonOdiwuor commented at 12:34 PM on February 20, 2025:

I have reverted the extraction, is this still worth removing?

l0rinc commented at 3:38 PM on December 20, 2024: contributor

I wanted to see what the π pull request contains, ended up reviewing it. Could you please specify in the description of how the reviewers should test the code and why the extraction was necessary besides the absolute file computation?

pablomartin4btc commented at 4:42 PM on February 19, 2025: member

tACK 318b2a2f90dfd8ae20beca58e55e6d240a7f3d27

<details> <summary>I've managed to reproduce the issue in <code>master</code> and this PR fixes it.</summary>

script:

cat shell.py 
from pathlib import Path
import sys
REPO = Path(sys.argv.pop())
sys.path.insert(0, f"{REPO / 'build' / 'test' / 'functional'}")
from test_framework.test_shell import TestShell

TestShell().setup(num_nodes=1, setup_clean_chain=True)
TestShell().shutdown()

master run:


python3 shell.py .
Traceback (most recent call last):
  File "/home/pablo/workspace/bitcoin/shell.py", line 7, in <module>
    TestShell().setup(num_nodes=1, setup_clean_chain=True)
  File "/home/pablo/workspace/bitcoin/build/test/functional/test_framework/test_shell.py", line 78, in __new__
    TestShell.instance = TestShell.__TestShell(tests_directory / "testshell_dummy.py")
  File "/home/pablo/workspace/bitcoin/build/test/functional/test_framework/test_framework.py", line 106, in __init__
    self.parse_args(test_file)
  File "/home/pablo/workspace/bitcoin/build/test/functional/test_framework/test_framework.py", line 221, in parse_args
    config.read_file(open(self.options.configfile))
FileNotFoundError: [Errno 2] No such file or directory: '/home/pablo/workspace/bitcoin/test/config.ini'

BrandonOdiwuor:testshell-fix run:


python3 shell.py .
2025-02-19T16:24:08.067000Z TestFramework (INFO): PRNG seed is: 5076166937161550343
2025-02-19T16:24:08.067000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_9e2y3t09
2025-02-19T16:24:08.374000Z TestFramework (INFO): Stopping nodes
2025-02-19T16:24:08.525000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_9e2y3t09 on exit
2025-02-19T16:24:08.525000Z TestFramework (INFO): Tests successful

</details> [@BrandonOdiwuor](/bitcoin-bitcoin/contributor/brandonodiwuor/) please address pending feedback when's possible. Thanks!

BrandonOdiwuor force-pushed on Feb 20, 2025

DrahtBot added the label Tests on Feb 20, 2025

test: fix TestShell initialization and reset() 303f8cca05

BrandonOdiwuor force-pushed on Feb 20, 2025

BrandonOdiwuor commented at 12:38 PM on February 20, 2025: contributor

@l0rinc I have added instructions on how to test on the PR description

The reason for the extraction of the dummy_testshell_file initially was to share it with the reset() function to fix #31131, I have however reverted the change to avoid using global variable

BrandonOdiwuor requested review from l0rinc on Feb 20, 2025

BrandonOdiwuor requested review from pinheadmz on Feb 20, 2025

BrandonOdiwuor requested review from pablomartin4btc on Feb 20, 2025

pinheadmz approved

pinheadmz commented at 4:25 PM on February 20, 2025: member

ACK 303f8cca0568d2ca77189c4eccdfc7a6913c958d

Reviewed changes since last ACK, re-reviewed all changes, tested locally by creating a TestShell and sending RPC commands

<details><summary>Show Signature</summary>

pinheadmz's public key is on openpgp.org

</details>

fanquake commented at 7:48 PM on February 20, 2025: member

@willcl-ark want to take a look here?

theStack approved

theStack commented at 10:16 PM on February 21, 2025: contributor

Tested ACK 303f8cca0568d2ca77189c4eccdfc7a6913c958d

pablomartin4btc approved

pablomartin4btc commented at 5:01 PM on February 24, 2025: member

re-ACK 303f8cca0568d2ca77189c4eccdfc7a6913c958d

(reviewed changes since last ACK: update to build/test paths & reverted global var move)

fanquake merged this on Feb 25, 2025

fanquake closed this on Feb 25, 2025