Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

qa: Add emojis to test_runner path and wallet filename #13859

Merged
merged 1 commit into from
Aug 6, 2018

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Aug 3, 2018

Now that wallet filenames are properly quoted when used for rpc (#13823), we can add some unicode symbols to the test_runner path. Thus, the "extern" wallet that uses a full path has a unicode symbol in its name.

Should add unicode coverage to

  • listwallets
  • wallet.getwalletinfo
  • (un)loadwallet

@maflcko maflcko added the Tests label Aug 3, 2018
@fanquake
Copy link
Member

fanquake commented Aug 3, 2018

Concept ACK

I feel like this is an opportunity to use ₿ somewhere.

@skeees
Copy link
Contributor

skeees commented Aug 3, 2018

Concept ACK
stronger ACK if you mine the commit hash

@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 3, 2018

Note to 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.

@practicalswift
Copy link
Contributor

utACK fa5a3f51cc4b3dae2f134fae1745f392ca6e5e47

@maflcko maflcko force-pushed the Mf1808-qaWalletEmoji branch 2 times, most recently from cffffff to fa12af2 Compare August 3, 2018 12:15
@scravy
Copy link
Contributor

scravy commented Aug 3, 2018

utACK fa5a3f51cc4b3dae2f134fae1745f392ca6e5e47 – the macos build failed with a timeout, maybe restart the build?

@ken2812221
Copy link
Contributor

utACK 5e17777.
You have seven "7" in your commit hash.

@practicalswift
Copy link
Contributor

utACK 5e17777

@laanwj
Copy link
Member

laanwj commented Aug 6, 2018

💜-ACK 5e17777

@laanwj laanwj merged commit 5e17777 into bitcoin:master Aug 6, 2018
laanwj added a commit that referenced this pull request Aug 6, 2018
5e17777 qa: Create unicode tempdir in test_runner (MarcoFalke)

Pull request description:

  Now that wallet filenames are properly quoted when used for rpc (#13823), we can add some unicode symbols to the test_runner path. Thus, the "extern" wallet that uses a full path has a unicode symbol in its name.

  Should add unicode coverage to
  * `listwallets`
  * `wallet.getwalletinfo`
  * `(un)loadwallet`

Tree-SHA512: 1633fde56f8748df0cfef9c31a878c105dfaac85d1041b292261f44c4d40e96942aacbf7d6e839e8bbf979dc131d81c24ceb521e927fc8a5a71ba093f36b891b
@maflcko maflcko deleted the Mf1808-qaWalletEmoji branch August 6, 2018 16:27
@ryanofsky
Copy link
Contributor

ryanofsky commented Aug 7, 2018

This seems to be causing travis errors:

Traceback (most recent call last):
  File "test/functional/test_runner.py", line 616, in <module>
    main()
  File "test/functional/test_runner.py", line 233, 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)

https://travis-ci.org/bitcoin/bitcoin/jobs/402357380#L3946

I see same error locally if I run LC_ALL=C test/functional/test_runner.py, but perhaps this is expected.

@maflcko
Copy link
Member Author

maflcko commented Aug 7, 2018

@ryanofsky The travis error is due to a travis bug, where it wouldn't use the most recent travis yaml when a build is reset/rerun.

Locally, you'd have to pick a language setting that is UTF-8 or set LC_ALL=C.UTF-8

@scravy
Copy link
Contributor

scravy commented Aug 7, 2018

This is solved in #13863

@JeremyRubin
Copy link
Contributor

I like this and think it's fun... but is it possible we can revert this? It breaks clipboard copy-paste behavior on my system so it makes it a bit annoying to copy the directiory to examine the directory on failed test.

It's a minor annoyance, but idk if it's worth it?

@maflcko
Copy link
Member Author

maflcko commented Nov 7, 2019

@JeremyRubin We should absolutely test wallet behaviour for non-ascii directory and file paths. Especially given that it was broken in the past and some Bitcoin Core users have a non-ascii name.

@JeremyRubin
Copy link
Contributor

JeremyRubin commented Nov 7, 2019 via email

@maflcko
Copy link
Member Author

maflcko commented Nov 7, 2019

The test runner path defines the root dir for the temp test dirs, and some of our wallets use absolute paths that are not in the datadir or wallet dir. So the root dir is the only place where an emoji could be injected to make the path non-ascii.

Have you reported the clipboard bug upstream?

@ryanofsky
Copy link
Contributor

ryanofsky commented Nov 7, 2019

Have you reported the clipboard bug upstream?

I have the same problem with gnome terminal. They are really stubborn about their UI and in the past removed the ability to customize this behavior:

https://bugzilla.gnome.org/show_bug.cgi?id=730632

But maybe there is a way to customize it again:

https://askubuntu.com/questions/8413/can-i-specify-what-characters-set-the-double-click-selection-boundary-in-gnome-t

Though I think my real solution should just be to get a better terminal. Or maybe github could shadowban Marco's emojis so only he can be amused by them and everybody else will just wonder why he's so happy all the time.

@maflcko
Copy link
Member Author

maflcko commented Nov 7, 2019

Maybe we could add an OPT_OUT_OF_EMOJI environment variable that the test_runner checks before using emojis? The environment variable could be set in your .bashrc.

deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request May 5, 2020
Summary:
Backport of core [[bitcoin/bitcoin#13859 | PR13859]].

This looks funny, but is here to add some coverage of the utf-8 support.

Test Plan:
  ninja check-functional

Reviewers: #bitcoin_abc, majcosta

Reviewed By: majcosta

Differential Revision: https://reviews.bitcoinabc.org/D5958
UdjinM6 pushed a commit to UdjinM6/dash that referenced this pull request Jun 29, 2021
…lename

5e17777 qa: Create unicode tempdir in test_runner (MarcoFalke)

Pull request description:

  Now that wallet filenames are properly quoted when used for rpc (bitcoin#13823), we can add some unicode symbols to the test_runner path. Thus, the "extern" wallet that uses a full path has a unicode symbol in its name.

  Should add unicode coverage to
  * `listwallets`
  * `wallet.getwalletinfo`
  * `(un)loadwallet`

Tree-SHA512: 1633fde56f8748df0cfef9c31a878c105dfaac85d1041b292261f44c4d40e96942aacbf7d6e839e8bbf979dc131d81c24ceb521e927fc8a5a71ba093f36b891b
UdjinM6 pushed a commit to UdjinM6/dash that referenced this pull request Jun 29, 2021
…lename

5e17777 qa: Create unicode tempdir in test_runner (MarcoFalke)

Pull request description:

  Now that wallet filenames are properly quoted when used for rpc (bitcoin#13823), we can add some unicode symbols to the test_runner path. Thus, the "extern" wallet that uses a full path has a unicode symbol in its name.

  Should add unicode coverage to
  * `listwallets`
  * `wallet.getwalletinfo`
  * `(un)loadwallet`

Tree-SHA512: 1633fde56f8748df0cfef9c31a878c105dfaac85d1041b292261f44c4d40e96942aacbf7d6e839e8bbf979dc131d81c24ceb521e927fc8a5a71ba093f36b891b
UdjinM6 pushed a commit to UdjinM6/dash that referenced this pull request Jun 29, 2021
…lename

5e17777 qa: Create unicode tempdir in test_runner (MarcoFalke)

Pull request description:

  Now that wallet filenames are properly quoted when used for rpc (bitcoin#13823), we can add some unicode symbols to the test_runner path. Thus, the "extern" wallet that uses a full path has a unicode symbol in its name.

  Should add unicode coverage to
  * `listwallets`
  * `wallet.getwalletinfo`
  * `(un)loadwallet`

Tree-SHA512: 1633fde56f8748df0cfef9c31a878c105dfaac85d1041b292261f44c4d40e96942aacbf7d6e839e8bbf979dc131d81c24ceb521e927fc8a5a71ba093f36b891b
UdjinM6 pushed a commit to UdjinM6/dash that referenced this pull request Jun 29, 2021
…lename

5e17777 qa: Create unicode tempdir in test_runner (MarcoFalke)

Pull request description:

  Now that wallet filenames are properly quoted when used for rpc (bitcoin#13823), we can add some unicode symbols to the test_runner path. Thus, the "extern" wallet that uses a full path has a unicode symbol in its name.

  Should add unicode coverage to
  * `listwallets`
  * `wallet.getwalletinfo`
  * `(un)loadwallet`

Tree-SHA512: 1633fde56f8748df0cfef9c31a878c105dfaac85d1041b292261f44c4d40e96942aacbf7d6e839e8bbf979dc131d81c24ceb521e927fc8a5a71ba093f36b891b
UdjinM6 pushed a commit to UdjinM6/dash that referenced this pull request Jun 30, 2021
…lename

5e17777 qa: Create unicode tempdir in test_runner (MarcoFalke)

Pull request description:

  Now that wallet filenames are properly quoted when used for rpc (bitcoin#13823), we can add some unicode symbols to the test_runner path. Thus, the "extern" wallet that uses a full path has a unicode symbol in its name.

  Should add unicode coverage to
  * `listwallets`
  * `wallet.getwalletinfo`
  * `(un)loadwallet`

Tree-SHA512: 1633fde56f8748df0cfef9c31a878c105dfaac85d1041b292261f44c4d40e96942aacbf7d6e839e8bbf979dc131d81c24ceb521e927fc8a5a71ba093f36b891b
UdjinM6 pushed a commit to UdjinM6/dash that referenced this pull request Jul 1, 2021
…lename

5e17777 qa: Create unicode tempdir in test_runner (MarcoFalke)

Pull request description:

  Now that wallet filenames are properly quoted when used for rpc (bitcoin#13823), we can add some unicode symbols to the test_runner path. Thus, the "extern" wallet that uses a full path has a unicode symbol in its name.

  Should add unicode coverage to
  * `listwallets`
  * `wallet.getwalletinfo`
  * `(un)loadwallet`

Tree-SHA512: 1633fde56f8748df0cfef9c31a878c105dfaac85d1041b292261f44c4d40e96942aacbf7d6e839e8bbf979dc131d81c24ceb521e927fc8a5a71ba093f36b891b
UdjinM6 pushed a commit to UdjinM6/dash that referenced this pull request Jul 2, 2021
…lename

5e17777 qa: Create unicode tempdir in test_runner (MarcoFalke)

Pull request description:

  Now that wallet filenames are properly quoted when used for rpc (bitcoin#13823), we can add some unicode symbols to the test_runner path. Thus, the "extern" wallet that uses a full path has a unicode symbol in its name.

  Should add unicode coverage to
  * `listwallets`
  * `wallet.getwalletinfo`
  * `(un)loadwallet`

Tree-SHA512: 1633fde56f8748df0cfef9c31a878c105dfaac85d1041b292261f44c4d40e96942aacbf7d6e839e8bbf979dc131d81c24ceb521e927fc8a5a71ba093f36b891b
UdjinM6 pushed a commit to UdjinM6/dash that referenced this pull request Jul 2, 2021
…lename

5e17777 qa: Create unicode tempdir in test_runner (MarcoFalke)

Pull request description:

  Now that wallet filenames are properly quoted when used for rpc (bitcoin#13823), we can add some unicode symbols to the test_runner path. Thus, the "extern" wallet that uses a full path has a unicode symbol in its name.

  Should add unicode coverage to
  * `listwallets`
  * `wallet.getwalletinfo`
  * `(un)loadwallet`

Tree-SHA512: 1633fde56f8748df0cfef9c31a878c105dfaac85d1041b292261f44c4d40e96942aacbf7d6e839e8bbf979dc131d81c24ceb521e927fc8a5a71ba093f36b891b
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants