test: fix TestShell initialization (late follow-up for #30463)
#30714
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
|
cc @hebasto, maybe you have a better idea how to solve this; the current approach works but feels a bit hacky :) |
There was a problem hiding this comment.
I verified that this behavior occurs locally:
Python 3.12.4 (main, Jun 6 2024, 18:26:44) [Clang 15.0.0 (clang-1500.3.9.4)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> sys.path.insert(0, "/Users/abubakarismail/Desktop/bitcoin-dev/bitcoin/test/functional")
>>> from test_framework.test_shell import TestShell
>>> test = TestShell().setup()
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/Users/abubakarismail/Desktop/bitcoin-dev/bitcoin/test/functional/test_framework/test_shell.py", line 70, in __new__
TestShell.instance = TestShell.__TestShell()
^^^^^^^^^^^^^^^^^^^^^^^
TypeError: BitcoinTestFramework.__init__() missing 1 required positional argument: 'test_file'This PR bd7ce05 fixes this issue:
Python 3.12.4 (main, Jun 6 2024, 18:26:44) [Clang 15.0.0 (clang-1500.3.9.4)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> sys.path.insert(0, "/Users/abubakarismail/Desktop/bitcoin-dev/bitcoin/test/functional")
>>> from test_framework.test_shell import TestShell
>>> test = TestShell().setup()
2024-08-26T17:08:11.777000Z TestFramework (INFO): PRNG seed is: 8030963347355976194
2024-08-26T17:08:11.778000Z TestFramework (INFO): Initializing test directory /var/folders/dj/d8p8jhd172n7wnq81ryfl6rc0000gn/T/bitcoin_func_test_tzove0a_Tested ACK bd7ce05
I am neutral on the approach; I only did manual testing for now.
There was a problem hiding this comment.
tACK bd7ce05
I agree that the approach feels a bit hacky, but I can't see any other simple way to achieve the same result. This seems good enough, and the fix is pretty isolated :)
Github-Pull: bitcoin#30714 Rebased-From: bd7ce05
|
Backported to 28.x in #30762. |
b2a1379 depends: build libevent with -D_GNU_SOURCE (fanquake) 199bb09 test: fixing failing system_tests/run_command under some Locales (Jadi) 342baab test: Avoid intermittent timeout in p2p_headers_sync_with_minchainwork.py (MarcoFalke) 5577d5a test: fix `TestShell` initialization (late follow-up for #30463) (Sebastian Falbesoner) Pull request description: Backports: * #30714 * #30743 * #30761 * #30788 ACKs for top commit: willcl-ark: ACK b2a1379 achow101: ACK b2a1379 stickies-v: ACK b2a1379 Tree-SHA512: bf08ac0c613395def974a1b287345d4a64edc066c14f8c9f0184478b0e33e48333760eeb6e96b6b5fbafbb21b40d01875e3f526213a2734e226b2e111d71f3a3
Creating a
TestShellinstance as stated in the docs currently fails on master:Since #30463, BitcoinTestFramework instances expect the path of the calling test at construction, in order to find shared data like the configuration (config.ini) and the cache. Note that in contrast to actual functional tests, we can't simply pass
__file__here, as the test shell module sits within thetest_frameworksubfolder, so we have to navigate up to the parent directory and append some dummy test file name.On the long-term we should probably add some TestShell instantation smoke-test to detect issues like this early. As I'm not too familiar with the CI I'm not sure what is a good way to achieve this (a functional test obviously can't be used, as that's already a BitcoinTestFramework test in itself), but happy to take suggestions.