Skip to content

Conversation

@hebasto
Copy link
Member

@hebasto hebasto commented Apr 22, 2025

When using multi-config CMake generators, executable paths include per-config subdirectories, which require special handling in tests. Using dedicated environment variables to specify executable paths works well in such scenarios. However, the util_test_runner test sets these variables for the util/test_runner.py script unconditionally, which diverges from the approach used when running functional/test_runner.py.

This change makes the usage of the aforementioned environment variables uniform.

Also see: #32183 (comment).

When using multi-config CMake generators, executable paths include
per-config subdirectories, which require special handling in tests.
Using dedicated environment variables to specify executable paths
works well in such scenarios. However, the `util_test_runner` test
sets these variables for the `util/test_runner.py` script
unconditionally, which diverges from the approach used when running
`functional/test_runner.py`.

This change makes the usage of the aforementioned environment variables
uniform.
@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 22, 2025

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32324.

Reviews

See the guideline for information on the review process.

Type Reviewers
Concept ACK janb84

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:

  • #32697 (test: Turn util/test_runner into functional test by maflcko)

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.

@maflcko
Copy link
Member

maflcko commented Apr 22, 2025

Seems fine. An alternative could be to remove BUILDDIR and replace it with BINDIR, now that all bins are in one dir? BINDIR could include the multi-config subdir path element, if it exists.

@hebasto
Copy link
Member Author

hebasto commented Apr 22, 2025

Seems fine. An alternative could be to remove BUILDDIR and replace it with BINDIR, now that all bins are in one dir? BINDIR could include the multi-config subdir path element, if it exists.

The question is how to choose the configuration for testing when we built more than one config.

ctest has the -C option for that.

Copy link
Contributor

@janb84 janb84 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 3502107

This PR removes setting environment variables for the util test runner in the execution of the test and set the environment variables in more uniform way.

But isn't this PR superseded by PR #32697 which removes the test and therefor the need to make this adjustment ? (it's not merged yet but the PR links to this PR as a conflicting PR)

@maflcko
Copy link
Member

maflcko commented Jun 18, 2025

But isn't this PR superseded by PR #32697 which removes the test and therefor the need to make this adjustment ? (it's not merged yet but the PR links to this PR as a conflicting PR)

Correct, the other pr was created to fix all issues in one go, instead of having to go through them one-by-one.

@fanquake
Copy link
Member

Given #32697 already has a number of ACKs, I think we can just close this.

@fanquake fanquake closed this Jun 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants