test: fix TestShell initialization and reset()

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:

0$ python3
1>>> import sys
2>>> sys.path.insert(0, "./path/to/bitcoin/build/test/functional")
3>>> from test_framework.test_shell import TestShell
4>>> TestShell().setup(num_nodes=2, setup_clean_chain=True)
5>>> TestShell().shutdown()
6>>> 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:

 0#!/usr/bin/env python3
 1
 2# USAGE: shell.py /path/to/bitcoin/repo
 3
 4from pathlib import Path
 5import sys
 6REPO = Path(sys.argv.pop())
 7sys.path.insert(0, f"{REPO / 'build' / 'test' / 'functional'}")
 8from test_framework.test_shell import TestShell
 9
10TestShell().setup(num_nodes=1, setup_clean_chain=True)
11TestShell().shutdown()

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK 20d46f30232ddeb92b6158aa1240be065fb2f8c4
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmdgWAMACgkQ5+KYS2KJ
 7yTrzgQ//fY//TeYh+OmwVlUQqdaQm15OyCwnBYVbIS0INzIgzQ36pXWVPDmRuThC
 8cCRZTPRFlCklqYnuDEEtTTxrUbTWdAiXbbQI7fYiY+ZtwIHss+I/jlK8DuEHrWqP
 9mp+R7nVn4zWjYlli6Aytqg0vfkSRVMH/gzCSnL5qvt+bBAyworeWjYhG6WrdlOr5
10ViPP6uHR/KOLgVewZvIv5h+BSgBOhWhy5eQzU8lL2rS9NpBlEmlzgExnMRmdTQzQ
11azwQ2UyXvkvkjq430dQxZblL0Zcyxu4xzU3I11FR61QV9yloxomCQGJr8XAI90/g
12M8fJ07CO6TKVc4AYfyPogh+cPEN4VAeMvmbTrY2TF+LxCY4qFuKdXgafaRGPdHsk
13A3cDmKgn81Ys/zVdUXQ0k74r6Q2Ysaq2hf2dDghRlY5eWwZn3JWesfwIJRd/wQBj
14mu9uLiq2Ju0o1g2mLgejPe68GFMipyIExWHxlqsOogSkBOOZOZUdKIUjndn1YXZ4
15UtX9ttXASkRLizCHYcAlCG9K6Wzx/TmN6oRCBJJI11cqM1C/dV6GnA8+1PZmHLO1
163H0zSa4wUDbqMiHuzJMEht8jawu+Hx1lUSJCc3lSAlwzBAL47B5eMkOTJxKz0sfs
178Q5E8Nutwol+HLrcqWwUZ8UcgRADQX3KOF0HYWH2huw5rz6FWrc=
18=LVv/
19-----END PGP SIGNATURE-----

pinheadmz’s public key is on keybase

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
Stale ACK	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!

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK 318b2a2f90dfd8ae20beca58e55e6d240a7f3d27
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmdi7l8ACgkQ5+KYS2KJ
 7yTpK9A/+NJ4FSYMIQ313zWaQddHHYt2bcEY6veHmP0jeFhLOTi7Wx/sLIQSr9qN6
 8y8LjMXUu1+1P7PEAjQWVj+TYMapEb5SizZsDLD42J3YWz4648cUap+kgfqTQxoi2
 9aO+FMfjhwND6V7Cjo4lgKdHG7HYQRg8hLzGMMnOktKs2q2+DPv423fJQnh6q8F+F
10m81dX/4ftiGYzRrffjY8zMC2NIqZlZYQ6eEmdPmtL8pmVVvFm7OKtNKqUgXbnlYj
11ZCc0d9CjEXkPjvq5EfUWJtCBaZ7DfGyGO1UeK0kw/zKLAaAmjmS+S3wSAEGdOI5T
12w5WiiDzX/lkdHtPD6YoQ2JiEyQ3mabcL0AuHEQs7MIPNMh2VQlQriEYDA7hlRssi
13MvYEZwIQp9XmdG1YAQKYhGExqR8QPyTegX5vgeXc2xfKQsX01ioOFxCY8BdQgc2Y
14SRZZVx/T1xRM1Z7hSE0skI25VE/W3L6xeVozBIKpsZWubfMYjT1bbFG4eBohmpGU
15+9hIYQ1PHmF675Y3jlwSzRT0GVqJ2hiZo42aeA4LodOOLJwQJZYGSvg5+i7AzkE5
16dL5cpq178dUjsTONtAQuKgbLtWWiJqS+hzB1ZkVNjHCoAjpIlFXewcS3X1o3Os61
17c8Zs7lrs+P3dMZ2a85Shg6oYBE3/nPzwEI+Gq/8ZnFRkQA2yHS8=
18=Je0g
19-----END PGP SIGNATURE-----

pinheadmz’s public key is on keybase

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?

0dummy_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

script:

0cat shell.py 
1from pathlib import Path
2import sys
3REPO = Path(sys.argv.pop())
4sys.path.insert(0, f"{REPO / 'build' / 'test' / 'functional'}")
5from test_framework.test_shell import TestShell
6
7TestShell().setup(num_nodes=1, setup_clean_chain=True)
8TestShell().shutdown()

master run:

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

BrandonOdiwuor:testshell-fix run:

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

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

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK 303f8cca0568d2ca77189c4eccdfc7a6913c958d
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAme3V0wACgkQ5+KYS2KJ
 7yTqd1xAA1DppfrHcaHcdKp/HnsicUqMslnuQ6sKFoQK76EYEsQFTyjJi2QDno8iB
 8g4LI7wpfse3f6hYDuMn13joXmQ26wdlG/cV3r8RsquWMnxSoNoyJ2R8a8lZRehym
 9KxwDPivyfgOJPcU+iFERHBLy9eVKZH4n0n8nYtRElVncYoUbnMGvxDJ56aOmh6zw
10VI6NOpbgm0XBcokzvvThNR9zvU5PCJnzWKVR8YT8q7+fKN58EzMgu4M+54XFpMUS
11eX0ObFQl4zURKLCdSxfm3rtwLDCdsUplxNixEJZ2qRHk5yYD6cqTQrbW8M3EpRrI
128d8NNFYWb5ELDnHU+jVwaK1J9DSydoVmf33pwo9Mlj7/vTIM04d6HA+P7HF4x5vs
13Li+RRssSonSCM+gWwkS47J1dq1rkSp5YhK+3s9PZ49tLA4NI3QTONdzbdCZY9VDu
14x2Ccu9bHKombfwo+gbWYm6nv6bVXNZ8ocyTMuWi20xSUVJxQ0KPerGPCSEWQSBOC
15mv+kUQ2m7KJRWdh1Bf6V2muLELI9q/fRzAkVvetX9zG3M6QSEBJIXA/QHKvtC2gw
16MZWZe8rraNBLDQSukQsrxrPJ51SIvE1EoP9A/BND570b+vcC/aE62nzBGf2K+GJv
17rxign/38TrWNSVBCu0gVbHc4Wb2wW5936pOKI0VDMs/wSvBxvv8=
18=BuD1
19-----END PGP SIGNATURE-----

pinheadmz’s public key is on openpgp.org

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

test: fix TestShell initialization and reset() #31415

Code Coverage & Benchmarks

Reviews