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

hebasto · 2023-05-03T10:23:43Z

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 hebasto#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.

Make an out-of-source build:

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

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.

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

DrahtBot · 2023-05-03T10:23:46Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	stickies-v, MarcoFalke
Concept ACK	theuni

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

hebasto · 2023-05-03T10:26:31Z

Friendly ping @stickies-v :)

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.

test/functional/test_runner.py

hebasto · 2023-05-05T16:53:04Z

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

hebasto · 2023-05-05T17:00:41Z

Updated a310888 -> fdff215 (pr27561.01 -> pr27561.02, diff):

addressed @stickies-v's comment

theuni

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.

maflcko

lgtm

test/functional/test_runner.py

hebasto · 2023-05-19T10:45:31Z

Dropped non-required refactoring commit.

maflcko · 2023-05-19T11:27:47Z

lgtm ACK 342a0c8

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?

hebasto · 2023-05-19T11:37:20Z

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

We use file(CREATE_LINK ... COPY_ON_ERROR) . See hebasto@d6ab2fe.

maflcko · 2023-05-19T12:04:16Z

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.

hebasto · 2023-05-19T12:42:16Z

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?

The SeCreateSymbolicLinkPrivilege policy is disabled by default for non-administartors on Windows.
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.

maflcko · 2023-05-19T12:45:49Z

Anyway, should be unrelated to this pull request.

stickies-v · 2023-05-19T13:35:31Z

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.

bitcoin/test/functional/setup.py example:

from setuptools import setup, find_packages

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

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.

hebasto · 2023-05-19T15:43:08Z

Install the package in editable mode:

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

stickies-v

ACK 342a0c8 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.

test/functional/test_runner.py

This change allows `test_runner.py` to work from an out-of-source build directory using a symlink, a hard link or a copy on any platform.

hebasto · 2023-05-19T18:23:20Z

Updated 342a0c8 -> c44f3f2 (pr27561.03 -> pr27561.04, diff):

addressed @stickies-v's comment

stickies-v

re-ACK c44f3f2

maflcko

lgtm ACK c44f3f2 💸

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

…BITCOINUTIL` and `BITCOINTX` 4f2f615 test: Make `util/test_runner.py` honor `BITCOINUTIL` and `BITCOINTX` (Hennadii Stepanov) Pull request description: This PR is a continuation of changes to our testing frameworks (bitcoin/bitcoin#27554, bitcoin/bitcoin#27561) that allow them to work correctly in a multi-config build environment that is possible for [upcoming](bitcoin/bitcoin#25797) CMake-based build system. That means that built for different configurations binaries (e.g., "Debug" and "Release") can coexist in separated directories. The commit has been pulled from hebasto/bitcoin#15 and it seems [useful](hebasto/bitcoin#15 (comment)) by itself as: > I believe the rationale for allowing to drop in the executables via env var is to allow to test the guix-produced, or other third-party-produced executables... The current implementation of the `test/functional/test_framework/test_framework.py` script uses the same approach: https://github.com/bitcoin/bitcoin/blob/09351f51d279612973ecd76811dc075dff08209f/test/functional/test_framework/test_framework.py#L231-L246 ACKs for top commit: MarcoFalke: lgtm ACK 4f2f615 TheCharlatan: ACK 4f2f615 stickies-v: ACK 4f2f615 Tree-SHA512: 99ee9a727b266700649d8f2ec528dfaeb04a1e48f0cb1d4eeaece65917165be647c10c4548429a9e8b30d094597f67e860c1db03ac689ebb409b223ce1b63aa9

…ch tests for c44f3f2 test: Explicitly specify directory where to search tests for (Hennadii Stepanov) Pull request description: 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](bitcoin#25797). For the current master branch, this PR has no behaviour change. Required for hebasto#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 ``` ACKs for top commit: stickies-v: re-ACK bitcoin@c44f3f2 MarcoFalke: lgtm ACK c44f3f2 💸 Tree-SHA512: 622ff629080a55f76dd4c1dab6699de0e9f06b75da3315cd3b31b972ef4bde746458bf3e8a95e879b3c6a63be2368af70005a83f6a3c85c4f1ba5be51e91a61d

…TIL` and `BITCOINTX` 4f2f615 test: Make `util/test_runner.py` honor `BITCOINUTIL` and `BITCOINTX` (Hennadii Stepanov) Pull request description: This PR is a continuation of changes to our testing frameworks (bitcoin#27554, bitcoin#27561) that allow them to work correctly in a multi-config build environment that is possible for [upcoming](bitcoin#25797) CMake-based build system. That means that built for different configurations binaries (e.g., "Debug" and "Release") can coexist in separated directories. The commit has been pulled from hebasto#15 and it seems [useful](hebasto#15 (comment)) by itself as: > I believe the rationale for allowing to drop in the executables via env var is to allow to test the guix-produced, or other third-party-produced executables... The current implementation of the `test/functional/test_framework/test_framework.py` script uses the same approach: https://github.com/bitcoin/bitcoin/blob/09351f51d279612973ecd76811dc075dff08209f/test/functional/test_framework/test_framework.py#L231-L246 ACKs for top commit: MarcoFalke: lgtm ACK 4f2f615 TheCharlatan: ACK 4f2f615 stickies-v: ACK 4f2f615 Tree-SHA512: 99ee9a727b266700649d8f2ec528dfaeb04a1e48f0cb1d4eeaece65917165be647c10c4548429a9e8b30d094597f67e860c1db03ac689ebb409b223ce1b63aa9

DrahtBot added the Tests label May 3, 2023

hebasto mentioned this pull request May 3, 2023

build: Add CMake-based build system (6 of N) hebasto/bitcoin#15

Merged

stickies-v reviewed May 5, 2023

View reviewed changes

test/functional/test_runner.py Show resolved Hide resolved

hebasto force-pushed the 230503-path branch from a310888 to fdff215 Compare May 5, 2023 16:58

theuni reviewed May 18, 2023

View reviewed changes

maflcko approved these changes May 19, 2023

View reviewed changes

test/functional/test_runner.py Outdated Show resolved Hide resolved

hebasto force-pushed the 230503-path branch from fdff215 to 342a0c8 Compare May 19, 2023 10:44

maflcko approved these changes May 19, 2023

View reviewed changes

stickies-v approved these changes May 19, 2023

View reviewed changes

test/functional/test_runner.py Show resolved Hide resolved

test: Explicitly specify directory where to search tests for

c44f3f2

This change allows `test_runner.py` to work from an out-of-source build directory using a symlink, a hard link or a copy on any platform.

hebasto force-pushed the 230503-path branch from 342a0c8 to c44f3f2 Compare May 19, 2023 18:21

stickies-v approved these changes May 19, 2023

View reviewed changes

DrahtBot requested a review from maflcko May 19, 2023 18:49

maflcko reviewed May 22, 2023

View reviewed changes

fanquake merged commit 5421dc3 into bitcoin:master May 22, 2023

hebasto deleted the 230503-path branch May 22, 2023 09:24

hebasto mentioned this pull request May 22, 2023

test: Make util/test_runner.py honor BITCOINUTIL and BITCOINTX #27717

Merged

bitcoin locked and limited conversation to collaborators May 21, 2024

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

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

Uh oh!

Conversation

hebasto commented May 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Uh oh!

hebasto commented May 3, 2023

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hebasto commented May 5, 2023

Uh oh!

hebasto commented May 5, 2023

Uh oh!

theuni left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hebasto commented May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented May 19, 2023

Uh oh!

hebasto commented May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented May 19, 2023

Uh oh!

hebasto commented May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented May 19, 2023

Uh oh!

stickies-v commented May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented May 19, 2023

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hebasto commented May 19, 2023

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hebasto commented May 3, 2023 •

edited

Loading

DrahtBot commented May 3, 2023 •

edited

Loading

hebasto commented May 19, 2023 •

edited

Loading

hebasto commented May 19, 2023 •

edited

Loading

hebasto commented May 19, 2023 •

edited

Loading

stickies-v commented May 19, 2023 •

edited

Loading