test: Fix system_tests/run_command test #32601

1. 
   hebasto commented at 12:51 pm on May 23, 2025: member

   This PR addresses two issues related to the “Return non-zero exit code, with error message for stderr” check in the system_tests/run_command test:

   1. The test now checks the exact exit code, which must be 1.
   2. The process invocation string is removed from the exception message before checking the stderr output, as the latter is a substring of the former.

   This is an alternative to #32577 without introducing a shell wrapper and altering quotation in src/external_signer.cpp.

   The main idea is to avoid parsing the problematic sh -c 'echo err 1>&2 && false' command with subprocess::Popen.

   In the past, this check used:https://github.com/bitcoin/bitcoin/blob/5abb9b1af49be9024f95fa2f777285c531785d85/src/test/system_tests.cpp#L56-L58

   which was problematic due to its dependence on the system locale.

   Fixes #32574.

2. test: Fix `system_tests/run_command` test

   This change addresses two issues for the "Return non-zero exit code,
   with error message for stderr" case:
   1. The test now checks the exact exit code, which must be 1.
   2. The process invocation string is removed from the exception message
      before checking the stderr output, as the latter is a substring of
      the former.

   7db2aba469
3. hebasto added the label Tests on May 23, 2025
4. 
   DrahtBot commented at 12:51 pm on May 23, 2025: contributor

   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/32601.

   Reviews

   See the guideline for information on the review process. A summary of reviews will appear here.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #32577 (subprocess: Let shell parse command on non-Windows systems by hebasto)

   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.

5. DrahtBot added the label CI failed on May 23, 2025
6. 
   DrahtBot commented at 12:58 pm on May 23, 2025: contributor

   🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/42784049574 LLM reason (✨ experimental): The CI failure is due to a missing export LC_ALL=C statement in a shell script, as reported by a lint test.

   Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

   • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

   • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

   • An intermittent issue.

   Leave a comment here, if you need help tracking down a confusing failure.

7. in src/test/CMakeLists.txt:129 in 7db2aba469

   124@@ -125,6 +125,9 @@ add_executable(test_bitcoin
   125   validationinterface_tests.cpp
   126   versionbits_tests.cpp
   127 )
   128+set_property(SOURCE system_tests.cpp PROPERTY
   129+  COMPILE_DEFINITIONS PRINT_ERR_AND_FAIL_SCRIPT_PATH="${CMAKE_CURRENT_SOURCE_DIR}/print_err_and_fail.sh"
   

   ---

   ---

   

   maflcko commented at 1:02 pm on May 23, 2025:

   This will just re-introduce the bugs fixed in #31542, no?

   ---

   

   maflcko commented at 1:03 pm on May 23, 2025:

   A better option would be to use the test’s temp dir as scratch space

   ---

   

   hebasto commented at 1:05 pm on May 23, 2025:

   This will just re-introduce the bugs fixed in #31542, no?

   Oops, forgot about it :facepalm:

   ---

   

   hebasto commented at 2:41 pm on May 23, 2025:

   A better option would be to use the test’s temp dir as scratch space

   That doesn’t work with the currently used std::string overload of subprocess::Popen() because of to the space in the full path. Is using fs::temp_directory_path() acceptable?

   ---

   

   maflcko commented at 2:59 pm on May 23, 2025:

   Does quoting not work?

   ---

   

   hebasto commented at 3:00 pm on May 23, 2025:

   Does quoting not work?

   Not for me.

   ---

   

   hebasto commented at 3:03 pm on May 23, 2025:

   subprocess uses space and tab as delimiters:https://github.com/bitcoin/bitcoin/blob/2df824f4e62b6bc569044819cd64f66f3839ba13/src/util/subprocess.h#L307-L334

   ---

   

   maflcko commented at 3:30 pm on May 23, 2025:

   Then it seems like it should be fixed in RunCommandParseJSON, because real users could run into the bug as well?

   RunCommandParseJSON should just take a string and then pass it to the shell as-is?

   ---

   

   hebasto commented at 3:35 pm on May 23, 2025:

   That was the approach in #32577.

   ---

   

   maflcko commented at 3:58 pm on May 23, 2025:

   Yes, but it is unclear to me why it was closed, when it looks like the correct approach

   ---

   

   hebasto commented at 5:21 pm on May 23, 2025:

   Yes, but it is unclear to me why it was closed, when it looks like the correct approach

   Reopened.

8. hebasto marked this as a draft on May 23, 2025

Contributors
hebasto DrahtBot maflcko

Labels
Tests CI failed