test: Explicitly specify directory where to search tests for #27561

For out-of-source builds, the test/functional/test_runner.py is supposed to be run from the build directory which allows it to pick the test/config.ini file generated by the build system. Currently, it works accidently for the following reasons:

• on POSIX systems, when running a created by Autoconf symlink to the test/functional/test_runner.py in the source directory, it actually has the source directory location in the sys.path.
• on Windows (the build_msvc directory) VS project puts and copies every build artifact into the source tree (which is wrong and ugly).

This PR makes test/functional/test_runner.py work from a build directory in any form (a symbolic link, a hard link, a copy) on all supported platforms, which is highly desirable in the upcoming CMake-based build system.

For the current master branch, this PR has no behaviour change.

Required for https://github.com/hebasto/bitcoin/pull/15.

Steps to reproduce the issue

While the issue is mostly specific to Windows and CMake builds, it is still possible to reproduce it on the current master branch.

1. Make an out-of-source build:

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

2. Note that Autoconf created a symbolic link test/functional/test_runner.py in the ../build directory:

$ ls -l test/functional/test_runner.py 
lrwxrwxrwx 1 hebasto hebasto 47 May  5 17:40 test/functional/test_runner.py -> ../../../bitcoin/test/functional/test_runner.py

which works flawlessly.

3. However, replacing this symbolic link with a hard link or a copy of test/functional/test_runner.py from the source tree will cause the following error:

$ cp ../bitcoin/test/functional/test_runner.py test/functional/test_runner.py
$ ls -l test/functional/test_runner.py
$ ./test/functional/test_runner.py 
Temporary test directory at /tmp/test_runner_₿_🏃_20230505_175104
Running Unit Tests for Test Framework Modules
E
======================================================================
ERROR: test_framework (unittest.loader._FailedTest)
----------------------------------------------------------------------
ImportError: Failed to import test module: test_framework
Traceback (most recent call last):
  File "/usr/lib/python3.10/unittest/loader.py", line 154, in loadTestsFromName
    module = __import__(module_name)
ModuleNotFoundError: No module named 'test_framework'


----------------------------------------------------------------------
Ran 1 test in 0.000s

FAILED (errors=1)
Early exiting after failure in TestFramework unit tests

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

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

Friendly ping @stickies-v :)

Would it be possible to provide instructions on replicating how to make this fail withoutsys.path.append(tests_dir)? Often times, messing with system.path is treating the symptoms when actually the package structure needs to be improved. May not be true in this case, though.

@stickies-v

Would it be possible to provide instructions on replicating how to make this fail withoutsys.path.append(tests_dir)?

Sure. I've updated the PR description with steps to replicate the failure.

Updated a3108880fac89d4472fe3b24d133e9645bed41cf -> fdff2156e1ebc11818f34759bff80e7d37dfff7d (pr27561.01 -> pr27561.02, diff):

• addressed @stickies-v's comment

Concept ACK for CMake.

The path.join change is easy, but I'll defer to someone more familiar with the test framework on the path change.

lgtm

Dropped non-required refactoring commit.

lgtm ACK 342a0c865d8b4b00a088af7b70b1ee0df1864f5c

This just adds a duplicate entry in the normal case, because the symlink from configure is already followed by python, see https://docs.python.org/3/library/sys.html#sys.path, so the duplicate should be harmless.

Didn't check what Cmake does. I presume it copies?

Didn't check what Cmake does. I presume it copies?

We use file(CREATE_LINK ... COPY_ON_ERROR) . See https://github.com/hebasto/bitcoin/pull/15/commits/d6ab2fe8c58b23445f4cd2d820eee94a4bfdbd25.

Any reason to change it? I presume this would lead to copies instead of hard links when the build dir is on a different storage device? symbolic links could be used to follow to a different storage device.

Any reason to change it? I presume this would lead to copies instead of hard links when the build dir is on a different storage device?

1. The SeCreateSymbolicLinkPrivilege policy is disabled by default for non-administartors on Windows.
2. Even with the SeCreateSymbolicLinkPrivilege enabled, this change is still required on Windows.

symbolic links could be used to follow to a different storage device.

OK. I'll revert to my previous variant which uses the file(CREATE_LINK ... COPY_ON_ERROR SYMBOLIC) command.

Anyway, should be unrelated to this pull request.

Sure. I've updated the PR description with steps to replicate the failure.

Thanks, this helped a lot with my understanding.

I'm not a huge fan of the build system leaking into the test code, if we can avoid it. Someone reading this without knowledge of the build system wouldn't understand why we're appending to sys.path, so at the very least I'd suggest adding some documentation and/or a link to this PR.

An alternative approach could be to add a bitcoin/test/functional/setup.py file to make the test framework an installable package. When installing the package in editable mode (-e, acts kind of like a symlink), that would prevent developers having to reinstall the package for every change made to the test framework.

<details> <summary>bitcoin/test/functional/setup.py example:</summary>

from setuptools import setup, find_packages

setup(
    name='bitcoin_functional_tests',
    version='0.1',
    packages=find_packages(),
)

</details>

Install the package in editable mode:

pip install -e ../bitcoin/test/functional/

For me, this fixes the problem with the steps you've provided to reproduce the issue. I'm very unfamiliar with the build process though, so this may not be the best solution either. It's definitely more pythonic, and as another upside makes it easier to use the test framework as a library.

Install the package in editable mode:

The drawback of such an approach is that it creates additional files in the source tree.

ACK 342a0c865d8b4b00a088af7b70b1ee0df1864f5c modulo improved documentation (left a suggestion).

The drawback of such an approach is that it creates additional files in the source tree.

You're right, I can't find a way to install the package without it affecting the source tree in one way or another. So the current approach is probably the best approach.

Updated 342a0c865d8b4b00a088af7b70b1ee0df1864f5c -> c44f3f231988dc05c4c7a8a96bc2e7b1a54da277 (pr27561.03 -> pr27561.04, diff):

• addressed @stickies-v's comment

re-ACK https://github.com/bitcoin/bitcoin/commit/c44f3f231988dc05c4c7a8a96bc2e7b1a54da277

lgtm ACK c44f3f231988dc05c4c7a8a96bc2e7b1a54da277 💸

<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: lgtm ACK c44f3f231988dc05c4c7a8a96bc2e7b1a54da277 💸
H4uXJb9dCevTXQH5BFAvev56kkzUC6o51P3Mw7DA5MkLu5BfuD8loNRLNiQngaa0PIbX0MUDxQ1sSHObwvcKBQ==

</details>