qa: Functional test improvements #30463

hebasto commented at 8:14 PM on July 16, 2024: member

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 commented at 8:14 PM on July 16, 2024: contributor

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.

DrahtBot added the label Tests on Jul 16, 2024

maflcko commented at 8:25 PM on July 16, 2024: member

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()

scripted-diff: Add `__file__` argument to `BitcoinTestFramework.init()`

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

a0473442d1

qa: Consider `cache` and `config.ini` relative to invocation directory

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.

9bf7ca6cad

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

hebasto force-pushed on Jul 16, 2024

hebasto commented at 9:07 PM on July 16, 2024: member

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 commented at 7:27 AM on July 17, 2024: member

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.

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

maflcko commented at 7:29 AM on July 17, 2024: member

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

maflcko added this to the milestone 28.0 on Jul 17, 2024

hebasto commented at 7:48 AM on July 17, 2024: member

@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 commented at 7:53 AM on July 17, 2024: member

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

maflcko commented at 8:13 AM on July 17, 2024: member

Tested with the steps I provided.

tested ACK a8e3af1a82dd584a1cc3ffbe587e66889f72e3c 🎨

<details><summary>Show signature</summary>

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==

</details>

hebasto commented at 8:17 AM on July 17, 2024: member

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

Sure thing. The PR description has been updated.

hebasto commented at 8:22 AM on July 17, 2024: member

Friendly ping @stickies-v :)

in test/functional/feature_asmap.py:137 in a8e3af1a82

 132 | @@ -133,7 +133,8 @@ def run_test(self):
 133 |          self.node = self.nodes[0]
 134 |          self.datadir = self.node.chain_path
 135 |          self.default_asmap = os.path.join(self.datadir, DEFAULT_ASMAP_FILENAME)
 136 | -        self.asmap_raw = os.path.join(os.path.dirname(os.path.realpath(__file__)), ASMAP)
 137 | +        base_dir = self.config["environment"]["SRCDIR"]
 138 | +        self.asmap_raw = os.path.join(base_dir, ASMAP)

maflcko commented at 8:30 AM on July 17, 2024:

question: Is there a reason why this needs to change, but the other os.path.dirname(os.path.realpath(__file__)) and Path(__file__).parents are fine?

hebasto commented at 8:52 AM on July 17, 2024:

Other cases point to files that reside in the build directory. Or do you have any particular example?

maflcko commented at 9:41 AM on July 17, 2024:

Yeah, but this one uses os.path.realpath(__file__) as well (so should be in the srcdir already), no?

With my steps to reproduce, it passes before and after this commit.

Do you have other steps to reproduce?

hebasto commented at 10:40 AM on July 17, 2024:

Can our previous conversation be helpful?

maflcko commented at 11:36 AM on July 17, 2024:

Maybe, but it would be good to clarify what of the previous conversation is relevant.

Are you saying that this is needed for Windows and can not be tested on Unix?

hebasto commented at 1:30 PM on July 17, 2024:

Please disregard my previous comments if they contradict this one. My apologies for any confusion.

On Windows, creating symbolic links requires a permission, which is disabled by default. Therefore, CMake creates hard links (or copies as a fallback). In that case, os.path.realpath(__file__) will point at a file in the build directory.

An example of an error can be observed here:

268/313 - feature_asmap.py failed, Duration: 2 s
stdout:
2024-07-17T11:26:07.749000Z TestFramework (INFO): PRNG seed is: 7216823379970217686
2024-07-17T11:26:07.780000Z TestFramework (INFO): Initializing test directory D:\a\_temp\test_runner_₿_🏃_20240717_110914\feature_asmap_42
2024-07-17T11:26:08.584000Z TestFramework (INFO): Test bitcoind with no -asmap arg passed
2024-07-17T11:26:09.525000Z TestFramework (INFO): Test bitcoind -asmap=<absolute path>
2024-07-17T11:26:09.629000Z TestFramework (ERROR): Unexpected exception caught during testing
Traceback (most recent call last):
  File "D:\a\bitcoin\bitcoin\build\test\functional\test_framework\test_framework.py", line 132, in main
    self.run_test()
  File "D:\a\bitcoin\bitcoin\build\test\functional\feature_asmap.py", line 139, in run_test
    self.test_asmap_with_absolute_path()
  File "D:\a\bitcoin\bitcoin\build\test\functional\feature_asmap.py", line 61, in test_asmap_with_absolute_path
    shutil.copyfile(self.asmap_raw, filename)
  File "C:\hostedtoolcache\windows\Python\3.12.4\x64\Lib\shutil.py", line 260, in copyfile
    with open(src, 'rb') as fsrc:
         ^^^^^^^^^^^^^^^
FileNotFoundError: [Errno 2] No such file or directory: 'D:\\a\\bitcoin\\bitcoin\\build\\test\\functional\\../../src/test/data/asmap.raw'

maflcko commented at 11:44 AM on July 18, 2024:

Ok, so it can be reproduced in Linux with hard links.

However, this leads back to my original question: Why not change the other places as well?

For me, they also fail:

$ ln    $PWD/../test/functional/p2p_dos_header_tree.py   ./test/functional/
$ ./test/functional/p2p_dos_header_tree.py 
2024-07-18T11:42:08.340000Z TestFramework (INFO): PRNG seed is: 5276922920789009906
2024-07-18T11:42:08.341000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_9ppp3fwq
2024-07-18T11:42:08.741000Z TestFramework (INFO): Read headers data
2024-07-18T11:42:08.742000Z TestFramework (ERROR): Unexpected exception caught during testing
Traceback (most recent call last):
  File "$build_dir/test/functional/test_framework/test_framework.py", line 132, in main
    self.run_test()
  File "$build_dir/./test/functional/p2p_dos_header_tree.py", line 37, in run_test
    with open(self.headers_file_path, encoding='utf-8') as headers_data:
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
FileNotFoundError: [Errno 2] No such file or directory: '$build_dir/test/functional/data/blockheader_testnet3.hex'

hebasto commented at 9:04 PM on July 18, 2024:

Ok, so it can be reproduced in Linux with hard links.

Right. But we do not use hard links on non-Windows systems.

However, this leads back to my original question: Why not change the other places as well?

In other places, os.path.realpath(__file__) is used in conjunction with relative paths in the test/functional/ directory. Soft links (on non-Windows systems) and hard links (on Windows) are created for every file in the test/functional/ directory recursively. Therefore, the created paths are valid.

feature_asmap.py is an exception because data/asmap.raw resides outside of the test/functional/ directory.

As an alternative, the data/asmap.raw file can be moved to somewhere within the test/functional/ directory.

maflcko commented at 8:11 AM on July 19, 2024:

In other places, os.path.realpath(__file__) is used in conjunction with relative paths in the test/functional/ directory.

Thank you for the explanation. It makes sense, given that cmake creates all links recursively in this dir.

However, this leads back to my original question: Why not change the other affected places?

This still fails for me (and I expect it to fail in the cmake branch as well):

$ ln     $PWD/../test/functional/interface_rest.py    ./test/functional/
$ ./test/functional/interface_rest.py --valgrind 
2024-07-19T08:07:39.275000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "$bld/test/functional/test_framework/test_framework.py", line 569, in start_nodes
    node.wait_for_rpc_connection()
  File "$bld/test/functional/test_framework/test_node.py", line 270, in wait_for_rpc_connection
    raise FailedToStartError(self._node_msg(
test_framework.test_node.FailedToStartError: [node 0] bitcoind exited with status 1 during initialization.
==831680== FATAL: can't open suppressions file "$bld/contrib/valgrind.supp"
************************

hebasto commented at 10:59 AM on July 19, 2024:

Your example creates a hard link on Linux, which is not what happens in the CMake-based build system.

Do you think we should adjust our functional tests in way to handle hard links on Linux?

maflcko commented at 11:06 AM on July 19, 2024:

Oh, I see. You are saying that valgrind is not supported on Windows and hard links in the test framework are not supported on Linux, so there is nothing to be done here?

maflcko commented at 11:11 AM on July 19, 2024:

(Resolving discussion for now. Thanks for explaining and sorry for the lengthy replies.)

Edit: I don't have the resolve permission, but yeah, this is resolved from my side.

stickies-v approved

stickies-v commented at 11:20 AM on July 17, 2024: contributor

ACK a8e3af1a82dd584a1cc3ffbe587e66889f72e3c7

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 commented at 8:22 AM on July 22, 2024: member

Discussions are resolved and this seems rfm with two reviews?

glozow commented at 11:08 AM on July 22, 2024: member

ACK a8e3af1a82dd584a1cc3ffbe587e66889f72e3c7, tested with the steps in op

glozow merged this on Jul 22, 2024

glozow closed this on Jul 22, 2024

hebasto deleted the branch on Jul 22, 2024

theStack referenced this in commit bd7ce05f9d on Aug 25, 2024

glozow referenced this in commit 1e48238700 on Aug 29, 2024

fanquake referenced this in commit 5577d5a3c0 on Aug 30, 2024

achow101 referenced this in commit fa46088440 on Sep 5, 2024

bitcoin locked this on Jul 22, 2025