test: add functional test for `TestShell` (matching doc example)

theStack commented at 0:31 am on October 6, 2025: contributor

This PR adds a functional framework test for the TestShell class. The primary motivation for this is to avoid that the example instructions for the interactive Python shell in test-shell.md get outdated or broken without noticing, a problem we had already several times in the past (see #26520, #27906, #31415). Having a copy is still not perfect, as docs and functional test have to be kept in sync, but I don’t expect this to be a problem in practice, assuming the hint in the functional test comment is hopefully noticed if changes are made.

Alternatively, the example instructions in test-shell.md could be removed with a hint to the functional test (I tend to prefer to keep the docs as-is though, showing the full REPL interaction).

The first commit contain two small removal fix-ups in test-shell.md regarding the result of the createwallet RPC call and the mentioning of the noshutdown option that was removed earlier (see #31620). Could be backported to v30.

DrahtBot added the label Tests on Oct 6, 2025

DrahtBot commented at 0:31 am on October 6, 2025: 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/33546.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	brunoerg, stratospher, pinheadmz

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

theStack force-pushed on Oct 6, 2025

maflcko commented at 6:19 am on October 6, 2025: member

Looks like the new test times out in CI?

theStack force-pushed on Oct 6, 2025

theStack commented at 8:15 am on October 6, 2025: contributor

Force-pushed with a fix to make the test also work for non-wallet builds. With this, all CI jobs should pass now.

Looks like the new test times out in CI?

Hmm, seems like the test just hangs whenever the TestShell instance is not shut down at the end. Wrapped it in an try: ... finally: test.shutdown() block in order to prevent time-outs if any exception would be thrown there.

brunoerg commented at 1:25 pm on October 9, 2025: contributor

Concept ACK

doc: test: update TestShell example instructions/options

The `createwallet` RPC doesn't return the empty passphrase
warning anymore if no passphrase was passed explicitly.
The `noshutdown` parameter key was removed in commit
fa0dc09b9002f0bcae63af6af8d37fb3e0040ef4, so remove it from
the table.

53874f7934

test: add functional test for `TestShell` (matching doc example) 57f7c68821

in test/functional/feature_framework_testshell.py:41 in 9babfb9331 outdated

36+            test.sync_blocks()
37+            assert_equal(test.nodes[1].getblockchaininfo()["blocks"], 101)
38+            assert_equal(test.nodes[0].getbalance(), Decimal('50.0'))
39+            test.nodes[0].log.info("Successfully mined regtest chain!")
40+    finally:
41+        test.shutdown()

brunoerg commented at 1:59 pm on October 9, 2025:

After the shutdown, we could reset test and check if num_nodes has been reset. E.g.:

0     finally:
1         test.shutdown()
2+        test.reset()
3+        assert test.num_nodes is None

theStack commented at 3:27 pm on October 9, 2025:

Thanks, done.

theStack force-pushed on Oct 9, 2025

brunoerg approved

brunoerg commented at 4:43 pm on October 9, 2025: contributor

ACK 57f7c68821d96cc096db624cb06b7a252d038300

in test/functional/test_runner.py:363 in 57f7c68821

359@@ -360,6 +360,7 @@
360     'rpc_getdescriptorinfo.py',
361     'rpc_mempool_info.py',
362     'rpc_help.py',
363+    'feature_framework_testshell.py',

stratospher commented at 7:09 am on October 15, 2025:

57f7c68: nit: would keeping it in something like TEST_FRAMEWORK_TESTSHELL_TESTS (similar to TEST_FRAMEWORK_UNIT_TESTS) be a better place since it’s a test framework infra test.

pinheadmz commented at 3:30 pm on October 27, 2025:

I think this counts as unit test? Could just go in TEST_FRAMEWORK_UNIT_TESTS IMO

theStack commented at 7:46 pm on October 27, 2025:

Thanks for the reviews! Leaving these suggestions about either introducing new TEST_FRAMEWORK_... constants or extending existing ones as topic for a different PR, since I’m not sure what is the right approach here. E.g. one could even argue in the other direction and suggest to just remove the TEST_FRAMEWORK_UNIT_TESTS constant, as it’s only used once in the list and doesn’t seem to serve any deeper purpose, unless I’m missing something.

I think this counts as unit test? Could just go in TEST_FRAMEWORK_UNIT_TESTS IMO

I’d say it doesn’t count as unit test, since it doesn’t test any functionality in isolation (which is usually done using Python’s unittest module), but involves node interaction, same as the other feature_framework_*.py tests. Doing that would also need some deeper changes to test_runner.py, as TEST_FRAMEWORK_UNIT_TESTS is currently a single string rather than a list, i.e. by changing we couldn’t place it directly in BASE_SCRIPTS anymore.

stratospher commented at 7:47 am on October 15, 2025: contributor

ACK 57f7c68.

pinheadmz approved

pinheadmz commented at 3:29 pm on October 27, 2025: member

ACK 57f7c68821d96cc096db624cb06b7a252d038300

Modulo @stratospher comment about adding to TEST_FRAMEWORK_UNIT_TESTS.

Reviewed changes in both commits, followed the example from the doc manually and tried a variety of setup options and testshell commands. Confirmed the changes to the doc (wallet name and noshutdown). Built and ran test on macos/arm64

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK 57f7c68821d96cc096db624cb06b7a252d038300
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmj/jzAACgkQ5+KYS2KJ
 7yTpONg/+KiufnMWMbTEO3W1bxizD/YEjGxz0ijgmA2O+WXorCV7fMSQcSsiN7Lye
 8yv8kgxzaagm8DCfU2fyniK7eb1HQZSVemiMbRHIk/ZnmOY5ODXl7v7WBl2iCvCOW
 9CLxxzHwsMHnPpIsA8LfrtZW9TnPn3vLZIN4qloMOI+Q1NDPnpdhKFPeFWWqBFoT1
10TvhUJ8LMrzk1+G5RvFPKb8WW6wP2RH5n3K8N8Is+zDZXMLtCO1xQhXJKqXCpakNd
11x2Tv8t7HXOA8/wKYTQzpewNtE/UKQafhlJ9lkcpAhgABOU/2DfGPzYLMgYAmOi+G
12bArdl9KQxAYXMaPEm4S50NXuEgKzfedruRk2wtg125OAd1UH4JbdAyt6WH5hfA7R
13pOL8DrVWxU1a3RlSxEwhAumA4Hn7uMrcYcOBrI5HPIuzdgB20PZc1LyQfNK0jOPX
14HG+PofZr7kU0VWLrqswyiE05oJNrt3riuS6BYt95QlLLGvOhStnUGTF8NdnuAiSg
15iMqlpOyO8btxD+h1bT2xucCpfoBAXlGXC0RUicpUicmJJCWQc9cZhG0p+9zdVOen
16wiAyCWk1b6lW2btXSFBEae844VWJWQQoCVbR9uF5sQuY6dAkuDatWOLUBwpMdvY2
17T3M4Rnm6tP8pUMCpf7XOF2yf9SNCgI9hNlV7ZYFeGFbw5B8hzwo=
18=2ZxZ
19-----END PGP SIGNATURE-----

pinheadmz’s public key is on openpgp.org

glozow merged this on Oct 28, 2025

glozow closed this on Oct 28, 2025

theStack deleted the branch on Oct 28, 2025

test: add functional test for TestShell (matching doc example) #33546

Code Coverage & Benchmarks

Reviews

test: add functional test for `TestShell` (matching doc example) #33546