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 https://github.com/bitcoin/bitcoin/issues/31131

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

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

test: fix TestShell initialization and reset() 318b2a2f90

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

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?

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

 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.

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

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"

in test/functional/test_framework/test_shell.py:78 in 318b2a2f90

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

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?

test: fix TestShell initialization and reset() #31415

Code Coverage & Benchmarks

Reviews