Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
May 23, 2023
Merged

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

merged 1 commit into from
May 23, 2023

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented May 22, 2023

This PR is a continuation of changes to our testing frameworks (#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 hebasto#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:

bitcoin/test/functional/test_framework/test_framework.py

Lines 231 to 246 in 09351f5

def set_binary_paths(self):
"""Update self.options with the paths of all binaries from environment variables or their default values"""
binaries = {
"bitcoind": ("bitcoind", "BITCOIND"),
"bitcoin-cli": ("bitcoincli", "BITCOINCLI"),
"bitcoin-util": ("bitcoinutil", "BITCOINUTIL"),
"bitcoin-wallet": ("bitcoinwallet", "BITCOINWALLET"),
}
for binary, [attribute_name, env_variable_name] in binaries.items():
default_filename = os.path.join(
self.config["environment"]["BUILDDIR"],
"src",
binary + self.config["environment"]["EXEEXT"],
)
setattr(self.options, attribute_name, os.getenv(env_variable_name, default=default_filename))

@hebasto
test: Make util/test_runner.py honor BITCOINUTIL and BITCOINTX
4f2f615 
This change allows to drop in the executables via environment variables
in the same way as for functional tests.
@DrahtBot
Copy link
Contributor

DrahtBot commented May 22, 2023 •
edited
Loading

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 DrahtBot added the Tests label May 22, 2023
@hebasto hebasto mentioned this pull request May 22, 2023
Merged
theuni
theuni reviewed May 22, 2023
View reviewed changes
Copy link
Member

@theuni theuni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK.

I initially complained about the approach here, but it seems it's just following the current convention.

@hebasto
Copy link
Member Author

hebasto commented May 23, 2023

As some progress is being made in the review of the coupled hebasto#15, friendly ping @MarcoFalke @stickies-v @ryanofsky :)

@maflcko
Copy link
Member

maflcko commented May 23, 2023

lgtm ACK 4f2f615

TheCharlatan
TheCharlatan approved these changes May 23, 2023
View reviewed changes
Copy link
Contributor

@TheCharlatan TheCharlatan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 4f2f615

The error reporting in this file is a bit opaque imo, but that is orthogonal to the work here.

stickies-v
stickies-v approved these changes May 23, 2023
View reviewed changes
Copy link
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 4f2f615

@fanquake fanquake merged commit 5ef2c1e into bitcoin:master May 23, 2023
@hebasto hebasto deleted the 230522-util branch May 23, 2023 12:28
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 23, 2023
@fanquake
Merge bitcoin#27717: test: Make util/test_runner.py honor `BITCOINU…
c222880 
…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
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Aug 16, 2023
@hebasto @luke-jr
test: Make util/test_runner.py honor BITCOINUTIL and BITCOINTX
fe2d636 
This change allows to drop in the executables via environment variables
in the same way as for functional tests.

Github-Pull: bitcoin#27717
Rebased-From: 4f2f615
@bitcoin bitcoin locked and limited conversation to collaborators May 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
3 more reviewers

@theuni theuni theuni left review comments

@TheCharlatan TheCharlatan TheCharlatan approved these changes

@stickies-v stickies-v stickies-v approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@hebasto @DrahtBot @maflcko @theuni @TheCharlatan @stickies-v @fanquake