test: Remove system_tests/run_command runtime dependencies
#33929
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33929. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsNo conflicts as of last run. |
There was a problem hiding this comment.
would be nice to fix the space error, but not sure how easy that is.
review ACK c996515 🚆
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: review ACK c996515a6dcb158d2d3aa88efc499ffff49bea79 🚆
tIPkSDEjaEB97BQIv8sM9m414fm5sxxOHHqLWNWGBf6fsHuOwabixwP9VIGmJHyf+ljEBjgD2gIKL2G+psZMAA==
|
|
||
| BOOST_AUTO_TEST_CASE(run_command) | ||
| { | ||
| const std::string test_executable = std::string(boost::unit_test::framework::master_test_suite().argv[0]).append(" --log_level=nothing --report_level=no --run_test=mock_process/"); |
There was a problem hiding this comment.
nit: This will fail with spaces in the dir:
$ ./a\ space/test_bitcoin -t system_tests
Running 1 test case...
unknown location(0): fatal error: in "system_tests/run_command": subprocess::CalledProcessError: execve failed: No such file or directory (2)
test/system_tests.cpp(29): last checkpoint
*** 1 failure is detected in the test module "Bitcoin Core Test Suite"
|
Concept ACK c996515 Nice approach using the test binary itself as a mock process. nit: This will fail if the path contains spaces: The executable path in |
@bensig I've seen you minimally reword a prior review (here and in other pull requests), which makes you look like an LLM bot. Review is needed and welcome, but it does not help to simply repeat prior comments, such as the motivation from the pull request description, or from other reviews. It would be better to only mention new findings or new points of view. |
You got it... I am still learning the ropes here. I did 21+ reviews this week so far... I was simply reiterating something that I verified.. I will minimize my ACKs to only include any verifications that I have done or new information. Thanks for the feedback. |
|
We're usually more interested to know that you have verified the change yourself (e.g. recreated the change locally, ended up with same changes; ran all tests/benchmarks locally; searches for similar PRs or occurrences where the change applies, ran the fuzzer for 36 hours; did an IBD against this change; tested it on a big-endian/32-bit/raspberry-pi/windows, etc), i.e. that there's PoW behind the ack. |
5 participants
system_testscurrently rely oncat,echo,falseandshbeing available inPATHat runtime.This PR:
The change is primarily motivated by my work on maintaining the
bitcoin-corepackage in Guix. It enables the removal of the existingbashandcoreutilsnative inputs, which in turn makes it possible to drop the implicit dependency onqtbase@5(see https://codeberg.org/guix/guix/pulls/4386#issuecomment-8613333).