Non-ASCII character support#45736

mertcanaltin · 2022-12-04T20:09:38Z

Is the character's ASCII code greater than 127? (In this case, the function returns true)
Is the character a number? (In this case, the function returns false)
Is the character a "#" symbol? (In this case, the function returns false)
If no conditions are met, the function returns true.

mertcanaltin · 2022-12-04T21:13:13Z

test

constisLiteralSymbol=(char)=>{constcode=char.charCodeAt(0);if(code>127){returntrue;}if(char>='0'&&char<=9){returnfalse;}if(code===35){returnfalse;}returntrue;}

output

isLiteralSymbol('أهلا')true

lib/internal/test_runner/tap_lexer.js

test/parallel/test-runner-tap-lexer.js

mertcanaltin · 2022-12-04T22:07:03Z

@anonrig I applied the changes, thank you very much for the review 🚀

test/parallel/test-runner-tap-lexer.js

lib/internal/test_runner/tap_lexer.js

anonrig · 2022-12-04T23:03:56Z

@manekinekko Can you review this?

MoLow

the change LGTM, but this seems to break quite a few tests

lib/internal/test_runner/tap_lexer.js

manekinekko · 2022-12-05T14:03:19Z

test/parallel/test-runner-tap-lexer.js

+assert.strictEqual(isLiteralSymbol('#'),false);
+assert.strictEqual(isLiteralSymbol('أ'),true);
+assert.strictEqual(isLiteralSymbol('ت'),true);
+assert.strictEqual(isLiteralSymbol('ث'),true);


Could you:
group these into 2 groups: literals (true) and non-literals (false)?
add more samples of non-Latin characters? you can cherry-pick from this list here.

@manekinekko thank you so much i will fly here 🚀

@manekinekko would that be healthy
{constliterals=['A','a','-','+','أ','ت','ث','讲','演','講'];constnonLiterals=['0','#','\\','+','-'];literals.forEach((literal)=>{assert.strictEqual(isLiteralSymbol(literal),true);});nonLiterals.forEach((nonLiteral)=>{assert.strictEqual(isLiteralSymbol(nonLiteral),false);});}

mertcanaltin · 2022-12-05T15:00:51Z

I'm so sorry I triggered you all to review, sorry for the extra notifications :/

lib/internal/test_runner/tap_lexer.js

test/parallel/test-runner-tap-lexer.js

mertcanaltin · 2022-12-05T16:51:14Z

@manekinekko I sent the edits thank you very much

BridgeAR

Should we really accept zero width characters as acceptable input? I would skip all of them. We also already have a function to check for these:

node/lib/internal/util/inspect.js

Lines 2388 to 2398 in ab064d1

constisZeroWidthCodePoint=(code)=>{
returncode<=0x1F||// C0 control codes
(code>=0x7F&&code<=0x9F)||// C1 control codes
(code>=0x300&&code<=0x36F)||// Combining Diacritical Marks
(code>=0x200B&&code<=0x200F)||// Modifying Invisible Characters
// Combining Diacritical Marks for Symbols
(code>=0x20D0&&code<=0x20FF)||
(code>=0xFE00&&code<=0xFE0F)||// Variation Selectors
(code>=0xFE20&&code<=0xFE2F)||// Combining Half Marks
(code>=0xE0100&&code<=0xE01EF);// Variation Selectors
};

mertcanaltin · 2022-12-07T11:23:51Z

I didn't accept zero-width characters @BridgeAR i will update the code like this

if(typeofchar!=='string'||util.inspect.isZeroWidthCodePoint(char)){returnfalse;}

can I do it like this?

cjihrig · 2022-12-12T14:47:30Z

@mertcanaltin I think that would be ok.

MoLow · 2023-02-08T09:11:10Z

CC @nodejs/test_runner @manekinekko additional reviews will be appreciated

MoLow · 2023-02-08T09:11:37Z

@anonrig can you please dismiss your review/approve?

nodejs-github-bot · 2023-02-18T16:33:14Z

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

nodejs-github-bot · 2023-02-18T18:10:18Z

Landed in 3c6547f

mertcanaltin · 2023-02-18T20:07:43Z

🎉

PR-URL: nodejs#45736 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

PR-URL: #45736 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

PR-URL: nodejs#45736 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

PR-URL: #45736 Backport-PR-URL: #46839 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

nodejs-github-bot added dont-land-on-v14.x needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Dec 4, 2022

anonrig requested changes Dec 4, 2022
View reviewed changes

lib/internal/test_runner/tap_lexer.js Outdated Show resolvedHide resolved
lib/internal/test_runner/tap_lexer.js Outdated Show resolvedHide resolved
test/parallel/test-runner-tap-lexer.js Outdated Show resolvedHide resolved

anonrig added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Dec 4, 2022

mertcanaltin requested a review from anonrig December 4, 2022 22:36

anonrig requested changes Dec 4, 2022
View reviewed changes

anonrig requested a review from MoLow December 4, 2022 23:03

MoLow mentioned this pull request Dec 5, 2022
fix: tap parser fails when test name includes non ASCII characters #45741
Closed

MoLow reviewed Dec 5, 2022
View reviewed changes

manekinekko reviewed Dec 5, 2022
View reviewed changes

mertcanaltin requested review from MoLow, anonrig and manekinekko and removed request for MoLow, anonrig and manekinekko December 5, 2022 14:59

manekinekko reviewed Dec 5, 2022
View reviewed changes

lib/internal/test_runner/tap_lexer.js Outdated Show resolvedHide resolved
lib/internal/test_runner/tap_lexer.js Outdated Show resolvedHide resolved
test/parallel/test-runner-tap-lexer.jsShow resolvedHide resolved

mertcanaltin requested a review from manekinekko December 5, 2022 16:58

BridgeAR reviewed Dec 6, 2022
View reviewed changes

mertcanaltin closed this Dec 7, 2022

mertcanaltin reopened this Dec 7, 2022

mertcanaltin force-pushed the bug-45706 branch from 78c70a1 to 4697163Compare February 8, 2023 08:52

test_runner: parse non-ascii character correctly
eeb6f02

mertcanaltin force-pushed the bug-45706 branch from e05f17f to eeb6f02Compare February 8, 2023 09:05

MoLow approved these changes Feb 8, 2023
View reviewed changes

MoLow removed the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Feb 8, 2023

anonrig approved these changes Feb 8, 2023
View reviewed changes

MoLow added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Feb 9, 2023

MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 18, 2023

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 18, 2023

MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 18, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 18, 2023

nodejs-github-bot merged commit 3c6547f into nodejs:mainFeb 18, 2023

mertcanaltin changed the title ~~[WIP] Non-ASCII character support~~Non-ASCII character supportFeb 18, 2023

MoLow pushed a commit to MoLow/node that referenced this pull request Feb 18, 2023
test_runner: parse non-ascii character correctly
5b64b4b
PR-URL: nodejs#45736 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

MylesBorins pushed a commit that referenced this pull request Feb 18, 2023
test_runner: parse non-ascii character correctly
c2d735e
PR-URL: #45736 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

MylesBorins mentioned this pull request Feb 19, 2023
v19.7.0 proposal #46725
Merged

MylesBorins pushed a commit that referenced this pull request Feb 20, 2023
test_runner: parse non-ascii character correctly
ddf5002
PR-URL: #45736 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

MoLow pushed a commit to MoLow/node that referenced this pull request Feb 25, 2023
test_runner: parse non-ascii character correctly
c23e601
PR-URL: nodejs#45736 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

MoLow pushed a commit to MoLow/node that referenced this pull request Feb 25, 2023
test_runner: parse non-ascii character correctly
8957729
PR-URL: nodejs#45736 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

MoLow added the backport-open-v18.x label Feb 25, 2023

MoLow pushed a commit to MoLow/node that referenced this pull request Feb 25, 2023
test_runner: parse non-ascii character correctly
730f11f
PR-URL: nodejs#45736 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

juanarbol pushed a commit that referenced this pull request Mar 3, 2023
test_runner: parse non-ascii character correctly
8bd3c0f
PR-URL: #45736 Backport-PR-URL: #46839 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

juanarbol mentioned this pull request Mar 3, 2023
V18.15.0 proposal #46920
Merged

juanarbol pushed a commit that referenced this pull request Mar 5, 2023
test_runner: parse non-ascii character correctly
894d711
PR-URL: #45736 Backport-PR-URL: #46839 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

	constisZeroWidthCodePoint=(code)=>{
	returncode<=0x1F\|\|// C0 control codes
	(code>=0x7F&&code<=0x9F)\|\|// C1 control codes
	(code>=0x300&&code<=0x36F)\|\|// Combining Diacritical Marks
	(code>=0x200B&&code<=0x200F)\|\|// Modifying Invisible Characters
	// Combining Diacritical Marks for Symbols
	(code>=0x20D0&&code<=0x20FF)\|\|
	(code>=0xFE00&&code<=0xFE0F)\|\|// Variation Selectors
	(code>=0xFE20&&code<=0xFE2F)\|\|// Combining Half Marks
	(code>=0xE0100&&code<=0xE01EF);// Variation Selectors
	};

Uh oh!

Non-ASCII character support#45736

Non-ASCII character support #45736

Uh oh!

Conversation

mertcanaltin commented Dec 4, 2022• edited by MoLow Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mertcanaltin commented Dec 4, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mertcanaltin commented Dec 4, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

anonrig commented Dec 4, 2022

Uh oh!

MoLow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

manekinekkoDec 5, 2022

Choose a reason for hiding this comment

Uh oh!

mertcanaltinDec 5, 2022

Choose a reason for hiding this comment

Uh oh!

mertcanaltinDec 5, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mertcanaltin commented Dec 5, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mertcanaltin commented Dec 5, 2022

Uh oh!

BridgeAR left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mertcanaltin commented Dec 7, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjihrig commented Dec 12, 2022

Uh oh!

MoLow commented Feb 8, 2023

Uh oh!

MoLow commented Feb 8, 2023

Uh oh!

nodejs-github-bot commented Feb 18, 2023

Uh oh!

nodejs-github-bot commented Feb 18, 2023

Uh oh!

mertcanaltin commented Feb 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

mertcanaltin commented Dec 4, 2022•
edited by MoLow
Loading

mertcanaltin commented Dec 4, 2022•
edited
Loading

mertcanaltinDec 5, 2022•
edited
Loading

BridgeAR left a comment •
edited
Loading

mertcanaltin commented Dec 7, 2022•
edited
Loading