test: Fix `system_tests/run_command` test #32601

hebasto · 2025-05-23T12:51:35Z

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:

The test now checks the exact exit code, which must be 1.
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:

bitcoin/src/test/system_tests.cpp

Lines 56 to 58 in 5abb9b1

    
           // Return non-zero exit code, with error message for stderr 
        
           const std::string command{"ls nosuchfile"}; 
        
           const std::string expected{"No such file or directory"};

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

Fixes #32574.

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.

DrahtBot · 2025-05-23T12:51:38Z

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.

DrahtBot · 2025-05-23T12:58:09Z

🚧 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.}

Hints

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.

maflcko · 2025-05-23T13:02:57Z

src/test/CMakeLists.txt

  versionbits_tests.cpp
 )
+set_property(SOURCE system_tests.cpp PROPERTY
+  COMPILE_DEFINITIONS PRINT_ERR_AND_FAIL_SCRIPT_PATH="${CMAKE_CURRENT_SOURCE_DIR}/print_err_and_fail.sh"


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

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

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

Oops, forgot about it 🤦

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?

Does quoting not work?

subprocess uses space and tab as delimiters:

bitcoin/src/util/subprocess.h

Lines 307 to 334 in 2df824f

/*!

* Function: split

* Parameters:

* [in] str : Input string which needs to be split based upon the

* delimiters provided.

* [in] deleims : Delimiter characters based upon which the string needs

* to be split. Default constructed to ' '(space) and '\t'(tab)

* [out] vector<string> : Vector of strings split at deleimiter.

*/

static inline std::vector<std::string>

split(const std::string& str, const std::string& delims=" \t")

{

std::vector<std::string> res;

size_t init = 0;

while (true) {

auto pos = str.find_first_of(delims, init);

if (pos == std::string::npos) {

res.emplace_back(str.substr(init, str.length()));

break;

}

res.emplace_back(str.substr(init, pos - init));

pos++;

init = pos;

}

return res;

}

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?

That was the approach in #32577.

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

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

Reopened.

fanquake · 2025-07-04T09:09:54Z

Probably close this now, if #32577 has been re-opened, and the approach here has been nacked for re-introducing bugs?

hebasto added the Tests label May 23, 2025

hebasto mentioned this pull request May 23, 2025

subprocess: Let shell parse command on non-Windows systems #32577

Draft

DrahtBot added the CI failed label May 23, 2025

maflcko reviewed May 23, 2025

View reviewed changes

hebasto marked this pull request as draft May 23, 2025 13:07

hebasto closed this Jul 4, 2025

	// Return non-zero exit code, with error message for stderr
	const std::string command{"ls nosuchfile"};
	const std::string expected{"No such file or directory"};

	/*!
	* Function: split
	* Parameters:
	* [in] str : Input string which needs to be split based upon the
	* delimiters provided.
	* [in] deleims : Delimiter characters based upon which the string needs
	* to be split. Default constructed to ' '(space) and '\t'(tab)
	* [out] vector<string> : Vector of strings split at deleimiter.
	*/
	static inline std::vector<std::string>
	split(const std::string& str, const std::string& delims=" \t")
	{
	std::vector<std::string> res;
	size_t init = 0;

	while (true) {
	auto pos = str.find_first_of(delims, init);
	if (pos == std::string::npos) {
	res.emplace_back(str.substr(init, str.length()));
	break;
	}
	res.emplace_back(str.substr(init, pos - init));
	pos++;
	init = pos;
	}

	return res;
	}

test: Fix system_tests/run_command test #32601

test: Fix system_tests/run_command test #32601

Uh oh!

Conversation

hebasto commented May 23, 2025

Uh oh!

DrahtBot commented May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

DrahtBot commented May 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fanquake commented Jul 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

test: Fix `system_tests/run_command` test #32601

test: Fix `system_tests/run_command` test #32601

DrahtBot commented May 23, 2025 •

edited

Loading