Skip to content

tools: allow test tap output to include Unicode characters - #47175

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
MoLow:allow-tap-to-include-utf
Mar 21, 2023
Merged

tools: allow test tap output to include Unicode characters#47175
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
MoLow:allow-tap-to-include-utf

Conversation

@MoLow

@MoLowMoLow commented Mar 20, 2023

Copy link
Copy Markdown
Member

Fixes#47173

the issue can be easily reproduced locally by adding sys.stdout.reconfigure(encoding='ascii') to the beginning of tools/test.py

@nodejs-github-botnodejs-github-bot added test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Mar 20, 2023
@MoLow

MoLow commented Mar 20, 2023

Copy link
Copy Markdown
MemberAuthor

CC @nodejs/python @richardlau

@richardlaurichardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 20, 2023
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 20, 2023
@nodejs-github-bot

nodejs-github-bot commented Mar 20, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/50494/

@targostargos added the fast-track PRs that do not need to wait for 48 hours to land. label Mar 20, 2023
@richardlau

richardlau commented Mar 20, 2023

Copy link
Copy Markdown
Member

Well this doesn't look quite right 😆:
https://ci.nodejs.org/job/node-test-commit-osx/51087/nodes=osx1015/console

17:24:54 b'ok 2119 parallel/test-readline-async-iterators'17:24:54 ---17:24:54 duration_ms: 10.52617:24:54 ...17:24:55 b'ok 2120 parallel/test-repl-history-perm'17:24:55 ---17:24:55 duration_ms: 0.11817:24:55 ...

Note the "b" prefix and single quotes. This results in:
https://ci.nodejs.org/job/node-test-commit-osx/51087/nodes=osx1015/
image

cclauss
cclauss approved these changes Mar 20, 2023
View reviewed changes

@cclausscclauss left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want the second representation (b'L\xc3\xbcsai') instead of the first (Lüsai) then I approve.

>>> import logging
>>> logger = logging.getLogger()
>>> logger.warning("Lüsai")
Lüsai
>>> logger.warning("Lüsai".encode("utf-8"))
b'L\xc3\xbcsai'

richardlau
richardlau requested changes Mar 20, 2023
View reviewed changes

@richardlaurichardlau left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#47175 (comment)

@cclausscclauss changed the title tools: allow test tap output to include unicode characterstools: allow test tap output to include Unicode charactersMar 20, 2023
@cclauss
cclauss self-requested a review March 20, 2023 18:13
@github-actionsgithub-actionsBot mentioned this pull request Mar 21, 2023
Open
19 tasks
@MoLow
MoLowforce-pushed the allow-tap-to-include-utf branch from 2b47470 to f83d168CompareMarch 21, 2023 07:17
@MoLowMoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 21, 2023
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 21, 2023
@nodejs-github-bot

nodejs-github-bot commented Mar 21, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/50510/

@MoLow
MoLow requested a review from richardlauMarch 21, 2023 08:02
@nodejs-github-bot

nodejs-github-bot commented Mar 21, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/50512/

@nodejsnodejs deleted a comment from github-actionsBotMar 21, 2023
@MoLowMoLow added fast-track PRs that do not need to wait for 48 hours to land. and removed fast-track PRs that do not need to wait for 48 hours to land. labels Mar 21, 2023
@github-actions

github-actionsBot commented Mar 21, 2023

Copy link
Copy Markdown
Contributor

Fast-track has been requested by @MoLow. Please 👍 to approve.

@MoLowMoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 21, 2023
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 21, 2023
@nodejs-github-bot
nodejs-github-bot merged commit f5eedf4 into nodejs:mainMar 21, 2023
@nodejs-github-bot

nodejs-github-bot commented Mar 21, 2023

Copy link
Copy Markdown
Collaborator

Landed in f5eedf4

@MoLow
MoLow deleted the allow-tap-to-include-utf branch March 21, 2023 17:28
This was referenced Mar 22, 2023
Open
Open
@RafaelGSSRafaelGSS mentioned this pull request Apr 3, 2023
Merged
RafaelGSS pushed a commit that referenced this pull request Apr 5, 2023
@MoLow@RafaelGSS
tools: allow test tap output to include unicode characters
546e866
PR-URL: #47175
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
@RafaelGSSRafaelGSS mentioned this pull request Apr 6, 2023
Merged
RafaelGSS pushed a commit that referenced this pull request Apr 7, 2023
@MoLow@RafaelGSS
tools: allow test tap output to include unicode characters
78ce8d3
PR-URL: #47175
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
@MoLow@danielleadams
tools: allow test tap output to include unicode characters
e0a00eb
PR-URL: #47175
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
@danielleadamsdanielleadams mentioned this pull request Jul 10, 2023
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@targostargostargos approved these changes
@cclausscclausscclauss approved these changes
@richardlaurichardlaurichardlau approved these changes

Assignees

No one assigned

Labels

fast-trackPRs that do not need to wait for 48 hours to land.testIssues and PRs related to the tests.toolsIssues and PRs related to the tools directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

pseudo-tty/test_runner_default_reporter breaks Python based test runner

5 participants

@MoLow@nodejs-github-bot@richardlau@targos@cclauss