Fix for test runner UnicodeDecodeError when utf-8 is not supported #14237

1. 
   sanket1729 commented at 6:58 am on September 17, 2018: contributor

   When attempting to create a directory using “/test_runner_₿🏃”, the following error is created.

   File "test/functional/test_runner.py", line 611, in <module> main() File "test/functional/test_runner.py", line 230, in main os.makedirs(tmpdir) File "/usr/lib/python3.6/os.py", line 220, in makedirs mkdir(name, mode) UnicodeEncodeError: 'ascii' codec can't encode character '\u20bf' in position 17: ordinal not in range(128)

   Since, the code already checks for UTF-8 encoding/decoding at a previous place, it makes sense to check for this string too.

   I am using docker container for testing bitcoin and it comes with a default sys.getfilesystemencoding() = ascii, which is how I stumbled into this.

   If this code will bloat the file, it might be better to use simple characters for directory name.

2. Fix for test runner UnicodeDecodeError when using ascii filesystem ca2634f9ca
3. fanquake added the label Tests on Sep 17, 2018
4. 
   DrahtBot commented at 8:49 am on September 17, 2018: member

   • #14275 (tests: Write the notification message to different files to avoid race condition in feature_notifications.py by ken2812221)
   • #14007 (tests: Run functional test on Windows and enable it on Appveyor by ken2812221)

   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. 
   ryanofsky commented at 4:40 pm on September 17, 2018: member

   I don’t think this change is a great idea because while it fixes one place where we are using non-ascii filenames, if we want to write new tests in the future that use non-ascii filenames (or non-ascii command line arguments), this will not be sufficient, and we’ll have to add similar workarounds in those places.

   It would seem better if you could add ENV LANG C.UTF-8 to your dockerfile, or update to python 3.7 where this should not be an issue (https://bugs.python.org/issue28180).

6. 
   sanket1729 commented at 5:12 pm on September 17, 2018: contributor

   There are some checks for Unicode before in the file here. So, if this is being enforced we should cleanup that code here to be consistent. https://github.com/bitcoin/bitcoin/blob/2d4749b366c4cae3da19017d6658f11349634303/test/functional/test_runner.py#L33. At the very least, we can change the dirname. We are introducing this dependency just for a directory name whereas at other places we checks for it . If the project ends up really using UTF-8 command line arguments, maybe it can be enforced at that time.
7. sanket1729 closed this on Sep 17, 2018

   ---

8. sanket1729 reopened this on Sep 17, 2018

   ---

9. 
   MarcoFalke commented at 5:17 pm on September 17, 2018: member

   @sanket1729 We allow unicode wallet file names (while certainly not encouraged, this should be possible today). Actually the the temp directory serves as a prefix for a wallet name.

   So I guess the best way forward is to enforce unicode early in test_runner and remove unneeded checks for unicode. (E.g. L33 that you linked to)

10. 
    sanket1729 commented at 5:30 pm on September 17, 2018: contributor

    Understood. Should I update this PR to remove that Unicode Check (since it is the only place it is being used)or create a separate issue and PR? Also, if test dependency not written anywhere in build instructions for dependencies, it should also be added there.
11. sanket1729 closed this on Sep 17, 2018

    ---

12. 
    MarcoFalke commented at 5:57 pm on September 17, 2018: member

    Jup, you can amend the commit and force push the changes, then change the pull request title as needed.

    If you also want to adjust the documentation: Feel free to do so at https://github.com/bitcoin/bitcoin/tree/master/test#running-tests-locally

13. MarcoFalke reopened this on Sep 17, 2018

    ---

14. 
    ryanofsky commented at 6:15 pm on September 17, 2018: member

    That stdout encoding check does looks broken. It should probably say "\u2713".encode(sys.stdout.encoding) instead of "\u2713".encode("utf_8").decode(sys.stdout.encoding).

    I don’t think the check should be necessarily removed though. While it’s true any unix system should be able pass through utf-8 encoded filenames to the filesystem, it’s not true that any terminal will happily display utf8 characters.

    If you want to do remove or modify the stdout check, I would open a new PR to avoid making the earlier comments in this PR unreadable.

15. MarcoFalke added the label Up for grabs on Sep 24, 2018
16. 
    DrahtBot commented at 8:21 pm on September 24, 2018: member
17. DrahtBot added the label Needs rebase on Sep 24, 2018
18. 
    MarcoFalke commented at 8:21 pm on September 24, 2018: member

    No activity in 7 days for a simple pull request. Closing for now. Let me know when you want to work on this again.
19. MarcoFalke closed this on Sep 24, 2018

    ---

20. laanwj removed the label Needs rebase on Oct 24, 2019
21. MarcoFalke locked this on Dec 16, 2021
22. MarcoFalke removed the label Up for grabs on Mar 22, 2022

Contributors
sanket1729 DrahtBot ryanofsky MarcoFalke

Labels
Tests