qa: Functional test improvements #30463

hebasto · 2024-07-16T20:14:02Z

This PR includes changes split from #30454. They improve the functional test framework, allowing users to run individual functional tests from the build directory in the new CMake-based build system.

This functionality is not available for out-of-source builds using the current Autotools-based build system, which always requires write permissions for the source directory. Nevertheless, this PR can be tested as suggested in #30463 (comment):

Make an out-of-source build:

$ ./autogen.sh
$ mkdir ../build && cd ../build
$ ../bitcoin/configure
$ make

Create a symlink in the build directory to a functional test:

$ ln --symbolic ../../../bitcoin/test/functional/wallet_disable.py ./test/functional/

Run this symlink:

$ ./test/functional/wallet_disable.py

The last command fails on the master branch:

Traceback (most recent call last):
  File "/home/hebasto/git/build/./test/functional/wallet_disable.py", line 31, in <module>
    DisableWalletTest().main()
    ^^^^^^^^^^^^^^^^^^^
  File "/home/hebasto/git/bitcoin/test/functional/test_framework/test_framework.py", line 106, in __init__
    self.parse_args()
  File "/home/hebasto/git/bitcoin/test/functional/test_framework/test_framework.py", line 210, in parse_args
    config.read_file(open(self.options.configfile))
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
FileNotFoundError: [Errno 2] No such file or directory: '/home/hebasto/git/bitcoin/test/config.ini'

and succeeds with this PR.

DrahtBot · 2024-07-16T20:14:06Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko, stickies-v, glozow

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#30207 (validation: Improve, document and test logic for chains building on invalid blocks by mzumsande)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

maflcko · 2024-07-16T20:25:14Z

The first commit could be split up as a scripted-diff? Also, formatting:

diff --git a/test/functional/interface_http.py b/test/functional/interface_http.py
index 567aa33cbe..dbdceb52d1 100755
--- a/test/functional/interface_http.py
+++ b/test/functional/interface_http.py
@@ -109 +109 @@ if __name__ == '__main__':
-    HTTPBasicsTest(__file__).main ()
+    HTTPBasicsTest(__file__).main()
--
diff --git a/test/functional/rpc_getchaintips.py b/test/functional/rpc_getchaintips.py
index 65a9583c22..1bd4413ce1 100755
--- a/test/functional/rpc_getchaintips.py
+++ b/test/functional/rpc_getchaintips.py
@@ -61 +61 @@ if __name__ == '__main__':
-    GetChainTipsTest(__file__).main ()
+    GetChainTipsTest(__file__).main()

-BEGIN VERIFY SCRIPT- sed -i -e 's/\s*().main\s*()/(__file__).main()/' $(git ls-files test/functional/*.py) sed -i -e 's/def __init__(self)/def __init__(self, test_file)/' test/functional/test_framework/test_framework.py -END VERIFY SCRIPT-

In CMake-based build system (1) `config.ini` is created in the build directory, and (2) `cache` must also be created in the same directory. This change enables running individual functional tests from the build directory.

hebasto · 2024-07-16T21:07:41Z

The first commit could be split up as a scripted-diff? Also, formatting:

diff --git a/test/functional/interface_http.py b/test/functional/interface_http.py
index 567aa33cbe..dbdceb52d1 100755
--- a/test/functional/interface_http.py
+++ b/test/functional/interface_http.py
@@ -109 +109 @@ if __name__ == '__main__':
-    HTTPBasicsTest(__file__).main ()
+    HTTPBasicsTest(__file__).main()
--
diff --git a/test/functional/rpc_getchaintips.py b/test/functional/rpc_getchaintips.py
index 65a9583c22..1bd4413ce1 100755
--- a/test/functional/rpc_getchaintips.py
+++ b/test/functional/rpc_getchaintips.py
@@ -61 +61 @@ if __name__ == '__main__':
-    GetChainTipsTest(__file__).main ()
+    GetChainTipsTest(__file__).main()

Thanks! Implemented.

maflcko · 2024-07-17T07:27:44Z

No behaviour changes for Autotools builds.

I don't think this is true. This is a bugfix.

Previously one could not run the tests in the autotools out-of-source build dir:

$ ./test/functional/wallet_disable.py 
bash: ./test/functional/wallet_disable.py: No such file or directory
$ ln --symbolic ../../../test/functional/wallet_disable.py ./test/functional/
$ ./test/functional/wallet_disable.py 
Traceback (most recent call last):
  File "./test/functional/wallet_disable.py", line 31, in <module>
    DisableWalletTest().main()
    ^^^^^^^^^^^^^^^^^^^
  File "$srcdir/test/functional/test_framework/test_framework.py", line 106, in __init__
    self.parse_args()
  File "$srcdir/test/functional/test_framework/test_framework.py", line 210, in parse_args
    config.read_file(open(self.options.configfile))
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
FileNotFoundError: [Errno 2] No such file or directory: '$srcdir/test/config.ini'

After this fix, it is possible.

~~Wrong: Also, the output of ./test/functional/test_runner.py --help changes, so this very much is a behavior change.~~

maflcko · 2024-07-17T07:29:13Z

Concept ACK on the behavior changes and the bugfix, but please properly describe the changes.

hebasto · 2024-07-17T07:48:19Z

@maflcko

Previously one could not run the tests in the autotools out-of-source build dir:

After this fix, it is possible.

That's true. But I did not consider this case as it requires a manual creation of a symlink to a test, which Autottols do not do, but CMake does.

maflcko · 2024-07-17T07:53:23Z

In any case, creating the symlinks seems like a good way to test this, no?

maflcko · 2024-07-17T08:13:56Z

Tested with the steps I provided.

tested ACK a8e3af1 🎨

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: tested ACK a8e3af1a82dd584a1cc3ffbe587e66889f72e3c 🎨
0l6ISxyQX7ey5ttupZ8f66fkchEymRF7a/bxNMR0+JttWLFMFvFkaYsxPISzikFFEUg17o1uHFGeuqHoN7xpBg==

hebasto · 2024-07-17T08:17:05Z

In any case, creating the symlinks seems like a good way to test this, no?

Sure thing. The PR description has been updated.

hebasto · 2024-07-17T08:22:40Z

Friendly ping @stickies-v :)

test/functional/feature_asmap.py

stickies-v

ACK a8e3af1

I'm able to run individual tests from an out-of-source build with the instructions provided. I tried running a bunch of other tests than wallet_disable too, including all the ones that reference __file__ and those seem to work fine too.

maflcko · 2024-07-22T08:22:53Z

Discussions are resolved and this seems rfm with two reviews?

glozow · 2024-07-22T11:08:12Z

ACK a8e3af1, tested with the steps in op

…#30463) bd7ce05 test: fix `TestShell` initialization (late follow-up for #30463) (Sebastian Falbesoner) Pull request description: Creating a `TestShell` instance as stated in the [docs](https://github.com/bitcoin/bitcoin/blob/master/test/functional/test-shell.md) currently fails on master: ``` $ python3 Python 3.10.13 (main, Mar 15 2024, 07:36:23) [Clang 16.0.6 ] on openbsd7 Type "help", "copyright", "credits" or "license" for more information. >>> import sys >>> sys.path.insert(0, "/home/thestack/bitcoin/test/functional") >>> from test_framework.test_shell import TestShell >>> test = TestShell().setup(num_nodes=2, setup_clean_chain=True) Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/home/thestack/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' ``` 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 the `test_framework` subfolder, 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. ACKs for top commit: ismaelsadeeq: Tested ACK bd7ce05 danielabrozzoni: tACK bd7ce05 brunoerg: ACK bd7ce05 Tree-SHA512: c3a2365e2cda48a233ee724673c490787981354914f33e10eadbbad9c68e8403d84c5551229a611401e743886539de380ba4bfcb77032b6c85731e3bbe962dc1

Github-Pull: bitcoin#30714 Rebased-From: bd7ce05

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

DrahtBot added the Tests label Jul 16, 2024

hebasto mentioned this pull request Jul 16, 2024

build: Introduce CMake-based build system #30454

Merged

hebasto added 3 commits July 16, 2024 22:06

qa: Do not assume running feature_asmap.py from source directory

a8e3af1

hebasto force-pushed the 240716-ftest branch from 28efce6 to a8e3af1 Compare July 16, 2024 21:07

maflcko added this to the 28.0 milestone Jul 17, 2024

maflcko reviewed Jul 17, 2024

View reviewed changes

test/functional/feature_asmap.py Show resolved Hide resolved

stickies-v approved these changes Jul 17, 2024

View reviewed changes

DrahtBot mentioned this pull request Jul 17, 2024

validation: Improve, document and test logic for chains building on invalid blocks #30207

Closed

hebasto mentioned this pull request Jul 18, 2024

cmake: Tracking issue for the main repo PRs hebasto/bitcoin#267

Closed

9 tasks

glozow merged commit 3a29ff5 into bitcoin:master Jul 22, 2024

hebasto deleted the 240716-ftest branch July 22, 2024 12:21

This was referenced Jul 24, 2024

Fix tiebreak when loading blocks from disk (and add tests for comparing chain ties) #29640

Merged

net: Favor peers from addrman over fetching seednodes #29605

Merged

mzumsande mentioned this pull request Jul 25, 2024

p2p: Increase inbound capacity for block-relay only connections #28463

Open

maflcko mentioned this pull request Aug 7, 2024

cmake: Copy tests to the build tree instead of linking them hebasto/bitcoin#309

Closed

theStack added a commit to theStack/bitcoin that referenced this pull request Aug 25, 2024

test: fix TestShell initialization (late follow-up for bitcoin#30463)

bd7ce05

theStack mentioned this pull request Aug 25, 2024

test: fix TestShell initialization (late follow-up for #30463) #30714

Merged

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Aug 30, 2024

test: fix TestShell initialization (late follow-up for bitcoin#30463)

5577d5a

Github-Pull: bitcoin#30714 Rebased-From: bd7ce05

bitcoin locked and limited conversation to collaborators Jul 22, 2025

qa: Functional test improvements #30463

qa: Functional test improvements #30463

Uh oh!

Conversation

hebasto commented Jul 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jul 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

maflcko commented Jul 16, 2024

Uh oh!

hebasto commented Jul 16, 2024

Uh oh!

maflcko commented Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jul 17, 2024

Uh oh!

hebasto commented Jul 17, 2024

Uh oh!

maflcko commented Jul 17, 2024

Uh oh!

maflcko commented Jul 17, 2024

Uh oh!

hebasto commented Jul 17, 2024

Uh oh!

hebasto commented Jul 17, 2024

Uh oh!

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jul 22, 2024

Uh oh!

glozow commented Jul 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hebasto commented Jul 16, 2024 •

edited

Loading

DrahtBot commented Jul 16, 2024 •

edited

Loading

maflcko commented Jul 17, 2024 •

edited

Loading