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

pull hebasto wants to merge 1 commits into bitcoin:master from hebasto:230522-util changing 1 files +5 −0

hebasto commented at 10:43 AM on May 22, 2023: member

This PR is a continuation of changes to our testing frameworks (https://github.com/bitcoin/bitcoin/pull/27554, #27561) that allow them to work correctly in a multi-config build environment that is possible for upcoming 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 https://github.com/hebasto/bitcoin/pull/15 and it seems useful 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

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

This change allows to drop in the executables via environment variables
in the same way as for functional tests.

4f2f615d13

DrahtBot commented at 10:43 AM on May 22, 2023: contributor



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 MarcoFalke, TheCharlatan, stickies-v

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.
DrahtBot added the label Tests on May 22, 2023
in test/util/test_runner.py:77 in 4f2f615d13
```
  73 | @@ -74,6 +74,11 @@ def bctest(testDir, testObj, buildenv):
  74 |      """
  75 |      # Get the exec names and arguments
  76 |      execprog = os.path.join(buildenv["BUILDDIR"], "src", testObj["exec"] + buildenv["EXEEXT"])
  77 | +    if testObj["exec"] == "./bitcoin-util":
```
theuni commented at 12:02 PM on May 22, 2023:

~Shouldn't this (and similar for the other) append EXEEXT to ./bitcoin-util here?~ Edit: Nm, I missed what was going on in the line above.

hebasto commented at 12:09 PM on May 22, 2023:

I guess not. This line is about parsing of the test/util/data/bitcoin-util-test.json file.

The correct file extension has already been added in the execprog variable above, or provided in an environment variable.
theuni commented at 12:03 PM on May 22, 2023: member

Concept ACK.

I initially complained about the approach here, but it seems it's just following the current convention.
hebasto commented at 10:14 AM on May 23, 2023: member

As some progress is being made in the review of the coupled https://github.com/hebasto/bitcoin/pull/15, friendly ping @MarcoFalke @stickies-v @ryanofsky :)
maflcko commented at 10:30 AM on May 23, 2023: member

lgtm ACK 4f2f615d1362afe92cabe9eab50087f8bfe454fd
TheCharlatan approved
TheCharlatan commented at 11:02 AM on May 23, 2023: contributor

ACK 4f2f615d1362afe92cabe9eab50087f8bfe454fd

The error reporting in this file is a bit opaque imo, but that is orthogonal to the work here.
stickies-v approved
stickies-v commented at 11:03 AM on May 23, 2023: contributor

ACK 4f2f615d1362afe92cabe9eab50087f8bfe454fd
fanquake merged this on May 23, 2023
fanquake closed this on May 23, 2023
hebasto deleted the branch on May 23, 2023
sidhujag referenced this in commit c2228804b5 on May 23, 2023
luke-jr referenced this in commit fe2d636f33 on Aug 16, 2023
bitcoin locked this on May 22, 2024

Type	Reviewers
ACK	MarcoFalke, TheCharlatan, stickies-v
Concept ACK	theuni

Contributors

Labels

Linked (view graph)

#25797 build: Add CMake-based build system #27554 test: Treat `bitcoin-wallet` binary in the same way as others #27561 test: Explicitly specify directory where to search tests for

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

Reviews