the run_command test under system_tests fails if the locale is anything other than English ones because results such as “No such file or directory” will be different under Non-English locales.
On the old version, a ls nonexistingfile was used to generate the error
output which is not ideal. In the current version we are using a Python one-liner
to generate a non 0 zero return value and “err” on stderr and check the
expected value against this.
Seems fine as a fix, but it seems incomplete, because the test may still fail if the file exists.
The test only wants to check the stderr and exit code, so instead of using ls, it may be easier to use a python command (https://github.com/bitcoin/bitcoin/pull/29868#discussion_r1740457415), or something similar.
echo success is never translated, because echo is just a tool to echo something, not to translate a passed string.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
Also, the commit message is wrong
echo successis never translated, becauseechois just a tool to echo something, not to translate a passed string.
you are right and thanks for mentioning it. fixed the message
The test only wants to check the stderr and exit code, so instead of using
ls, it may be easier to use a python command (#29868 (comment)), or something similar.
Agree with your solution. Python one-liner is cleaner and more reliable. Used that one so we do not need the Locale anymore. tests pass both in PT and C locale.
by the way since you are working on this in a larger scale, I can close my PR and wait for a full fix via yours.
the run_command test under system_tests fails if the locale is anything
other than English ones because results such as "No such file or directory"
will be different under Non-English locales.
On the old version, a `ls nonexistingfile` was used to generate the error
output which is not ideal. In the current version we are using a Python one-liner
to generate a non 0 zero return value and "err" on stderr and check the
expected value against this.
fixes #30608
review ACK 8289373
Please adjust the pull request description, now that the approach has changed: #30788 (comment)
Done. The PR description (https://github.com/bitcoin/bitcoin/pull/30788#issue-2500949706) is updated to reflect the changes in the commit message.
echo success isn’t translated), or outdated stuff of a previous commit. Please focus on the important and correct details only.
ACK ae48a22a3df086fb59843b7b814619ed5df7557b, tested on Ubuntu 24.04 by switching locale.
#30788#issue-2500949706
the run_command test under system_tests fails if the locale is anything other than US/UK/C (English ones) because it checks for things like
result.find_value("success");To prevent this, a
setenv("LC_ALL", "C", 1);is added to make sure that the tests are being run under C locale even if something else is set in the terminal.fixes #30608
Update: based on below discussions, the solution is changed. Now we are using a python one-liner to 1. return a non zero value and 2. output “err” on stderr and check the run against this which is more reliable than using “ls nonexistencefile” and hope for error. The commit message have been updated accordingly.
The second paragraph should be dropped, I guess.
The commit description looks fine, so why not copy-paste it and replace the pull request description?
just did. thanks.
The tests already depend on Python and ctest can already not be run without it. See also doc/dependencies.md:
0| [Python](https://www.python.org) (scripts, tests) | [3.9](https://github.com/bitcoin/bitcoin/pull/28211) |
If you find an alternative command, maybe based on GNU Coreutils that achieves the same, feel free to replace it.