test: Minor fix in test - locale in terminal #28286

1. 
   crywolf commented at 5:24 pm on August 17, 2023: none

   Running tests with non-English locale set in Linux terminal makes system tests (system_tests/run_command) fail. Setting the locale in failing test explicitly to ‘C’ fixes the failure.

   How to reproduce test failure: Setting LC_ALL=cs_CZ.UTF-8 (export LC_ALL=cs_CZ.UTF-8) in terminal and then running make check should be enough.

   Fix: My terminal is by default set to non-English locale and the test failed because it checks for English error message, but the terminal produces error message in the language set by locale. Then I manually set export LC_ALL=C in my terminal and all tests passed. So I explicitly set env variable ‘LC_ALL’ to ‘C’ in the code of failing test.

2. Minor fix in test - locale in terminal

   Running tests with non-English locale set in Linux terminal makes system tests (system_tests/run_command') fail.
   Setting the locale env variable 'LC_ALL' in failing test explicitly to 'C' fixes the failure.

   f2339eb44c
3. 
   DrahtBot commented at 5:24 pm on August 17, 2023: contributor

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

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Concept ACK	jonatack

   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:

   • #29489 (test: Remove Windows-specific code from system_tests/run_command by hebasto)
   • #28981 (Replace Boost.Process with cpp-subprocess 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.

4. DrahtBot added the label Tests on Aug 17, 2023
5. 
   crywolf commented at 5:25 pm on August 17, 2023: none

   During the workshop wit Gloria @glozow on Dev/Hack/Day (part of BTC Prague conference) my tests failed. Later that day I found what the problem was and now I am finally presenting my fix as an attempt to make my first PR in this repo.
6. 
   jonatack commented at 8:03 pm on August 17, 2023: contributor

   Concept ACK, though I’m unable to reproduce this on macOS, which makes sense per src/common/system.cpp#L66-73:

   0    // On most POSIX systems (e.g. Linux, but not BSD) the environment's locale
   1    // may be invalid, in which case the "C.UTF-8" locale is used as fallback.
   2#if !defined(WIN32) && !defined(MAC_OSX) && !defined(__FreeBSD__) && !defined(__OpenBSD__) && !defined(__NetBSD__)
   3    try {
   4        std::locale(""); // Raises a runtime error if current locale is invalid
   5    } catch (const std::runtime_error&) {
   6        setenv("LC_ALL", "C.UTF-8", 1);
   7    }
   

7. DrahtBot added the label CI failed on Aug 18, 2023
8. DrahtBot removed the label CI failed on Aug 18, 2023
9. DrahtBot added the label CI failed on Sep 3, 2023
10. DrahtBot removed the label CI failed on Sep 5, 2023
11. DrahtBot added the label CI failed on Oct 25, 2023
12. in src/test/system_tests.cpp:83 in f2339eb44c

    78@@ -79,6 +79,7 @@ BOOST_AUTO_TEST_CASE(run_command)
    79         const std::string command{"cmd.exe /c dir nosuchfile"};
    80         const std::string expected{wine_runtime ? "File not found." : "File Not Found"};
    81 #else
    82+        setenv("LC_ALL", "C", 1); // explicitly set locale env to get error message in English
    83         const std::string command{"ls nosuchfile"};
    

    ---

    ---

    

    maflcko commented at 9:19 pm on January 9, 2024:

    lgtm, but if the file exists, this will also fail.

    Might be better to use a pure command that does not depend on the env vars and filesystem state

13. 
    DrahtBot commented at 11:28 pm on February 28, 2024: contributor

    🐙 This pull request conflicts with the target branch and needs rebase.

14. DrahtBot added the label Needs rebase on Feb 28, 2024
15. 
    maflcko commented at 8:22 am on February 29, 2024: member

    Apologies for the slow review on this, and it eventually going stale. I’ll close for now, but the issue will still need a fix. Ideally like #28286 (review)

    Anyone is welcome to open a new pull request with the fix.

16. maflcko closed this on Feb 29, 2024

    ---

17. maflcko added the label Up for grabs on Feb 29, 2024

Contributors
crywolf DrahtBot jonatack maflcko

Labels
Tests Up for grabs Needs rebase CI failed