Non-ASCII character support #45736

mertcanaltin · Dec 4, 2022

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 · Dec 4, 2022

test

 const  isLiteralSymbol = (char) => {
    const code = char.charCodeAt(0);

    if (code > 127) {
      return true;
    }

    if (char >= '0' && char <= 9) {
      return false;
    }

    if (code === 35) {
      return false;
    }

    return true;
  }

output

isLiteralSymbol('أهلا')
true

lib/internal/test_runner/tap_lexer.js

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

mertcanaltin · Dec 4, 2022

@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 · Dec 4, 2022

@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 · Dec 5, 2022

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

{ const literals = ['A', 'a', '-', '+', 'أ', 'ت', 'ث', '讲', '演', '講']; const nonLiterals = ['0', '#', '\\', '+', '-']; literals.forEach((literal) => { assert.strictEqual(isLiteralSymbol(literal), true); }); nonLiterals.forEach((nonLiteral) => { assert.strictEqual(isLiteralSymbol(nonLiteral), false); }); }

mertcanaltin · Dec 5, 2022

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 · Dec 5, 2022

@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

    
           const isZeroWidthCodePoint = (code) => { 
        
             return code <= 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 · Dec 7, 2022

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

  if (typeof char !== 'string' || util.inspect.isZeroWidthCodePoint(char)) {
      return false;
    }

can I do it like this?

cjihrig · Dec 12, 2022

@mertcanaltin I think that would be ok.

mertcanaltin · Feb 8, 2023

hi i made some edits @anonrig , Thank you very much @MoLow

MoLow · Feb 8, 2023

@mertcanaltin can you squash your branch removing the merge commit?
i.e

git fetch upstream main
git reset --soft upstream/main
git commit -m "<COMMIT MESSAGE>"
git push --force-with-lease

lib/internal/test_runner/tap_lexer.js

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

MoLow · Feb 8, 2023

@mertcanaltin the first commit message should start with test_runner: prefix and describe the change.
the first commit is the commit that will be merged into the main branch so its name must be meaningful

MoLow · Feb 8, 2023

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

MoLow · Feb 8, 2023

@anonrig can you please dismiss your review/approve?

nodejs-github-bot · Feb 18, 2023

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

nodejs-github-bot · Feb 18, 2023

Landed in 3c6547f

mertcanaltin · Feb 18, 2023

🎉

PR-URL: nodejs#45736 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

PR-URL: #45736 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

nodejs-github-bot added dont-land-on-v14.x PRs that should not land on the v14.x-staging branch and should not be released in v14.x. needs-ci PRs that need a full CI run. test_runner labels Dec 4, 2022

anonrig requested changes Dec 4, 2022

View changes

lib/internal/test_runner/tap_lexer.js Outdated Show resolved Hide resolved

lib/internal/test_runner/tap_lexer.js Outdated Show resolved Hide resolved

test/parallel/test-runner-tap-lexer.js Outdated Show resolved Hide 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 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 changes

manekinekko reviewed Dec 5, 2022

View changes

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

manekinekko reviewed Dec 5, 2022

View changes

lib/internal/test_runner/tap_lexer.js Outdated Show resolved Hide resolved

lib/internal/test_runner/tap_lexer.js Outdated Show resolved Hide resolved

test/parallel/test-runner-tap-lexer.js Outdated Show resolved Hide resolved

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

BridgeAR reviewed Dec 6, 2022

View changes

mertcanaltin closed this Dec 7, 2022

mertcanaltin reopened this Dec 7, 2022

mertcanaltin force-pushed the bug-45706 branch from 1d3e74e to 7fce54d Compare February 8, 2023 08:32

MoLow reviewed Feb 8, 2023

View changes

lib/internal/test_runner/tap_lexer.js Outdated Show resolved Hide resolved

MoLow reviewed Feb 8, 2023

View changes

test/parallel/test-runner-tap-lexer.js Outdated Show resolved Hide resolved

MoLow reviewed Feb 8, 2023

View changes

test/parallel/test-runner-tap-lexer.js Outdated Show resolved Hide resolved

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

test_runner: parse non-ascii character correctly

eeb6f02

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

MoLow approved these changes Feb 8, 2023

View 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 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-actions bot 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:main Feb 18, 2023
54 checks passed

mertcanaltin changed the title ~~[WIP] Non-ASCII character support~~ Non-ASCII character support Feb 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 <yagiz@nizipli.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

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 <yagiz@nizipli.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

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 <yagiz@nizipli.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

Non-ASCII character support #45736

Non-ASCII character support #45736

mertcanaltin commented Dec 4, 2022 •

edited by MoLow

mertcanaltin commented Dec 4, 2022 •

edited

mertcanaltin commented Dec 4, 2022

anonrig commented Dec 4, 2022

MoLow left a comment

manekinekko Dec 5, 2022

mertcanaltin Dec 5, 2022

mertcanaltin Dec 5, 2022 •

edited

mertcanaltin commented Dec 5, 2022

mertcanaltin commented Dec 5, 2022

BridgeAR left a comment •

edited

mertcanaltin commented Dec 7, 2022 •

edited

cjihrig commented Dec 12, 2022

mertcanaltin commented Feb 8, 2023

MoLow commented Feb 8, 2023

MoLow commented Feb 8, 2023

MoLow commented Feb 8, 2023

MoLow commented Feb 8, 2023

nodejs-github-bot commented Feb 18, 2023

nodejs-github-bot commented Feb 18, 2023

mertcanaltin commented Feb 18, 2023

	const isZeroWidthCodePoint = (code) => {
	return code <= 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
	};

Jan	FEB	Mar
	23
2022	2023	2024

Non-ASCII character support #45736

Non-ASCII character support #45736

Conversation

mertcanaltin commented Dec 4, 2022 • edited by MoLow

mertcanaltin commented Dec 4, 2022 • edited

mertcanaltin commented Dec 4, 2022

anonrig commented Dec 4, 2022

MoLow left a comment

Choose a reason for hiding this comment

manekinekko Dec 5, 2022

Choose a reason for hiding this comment

mertcanaltin Dec 5, 2022

Choose a reason for hiding this comment

mertcanaltin Dec 5, 2022 • edited

Choose a reason for hiding this comment

mertcanaltin commented Dec 5, 2022

mertcanaltin commented Dec 5, 2022

BridgeAR left a comment • edited

Choose a reason for hiding this comment

mertcanaltin commented Dec 7, 2022 • edited

cjihrig commented Dec 12, 2022

mertcanaltin commented Feb 8, 2023

MoLow commented Feb 8, 2023

MoLow commented Feb 8, 2023

MoLow commented Feb 8, 2023

MoLow commented Feb 8, 2023

nodejs-github-bot commented Feb 18, 2023

nodejs-github-bot commented Feb 18, 2023

mertcanaltin commented Feb 18, 2023

mertcanaltin commented Dec 4, 2022 •

edited by MoLow

mertcanaltin commented Dec 4, 2022 •

edited

mertcanaltin Dec 5, 2022 •

edited

BridgeAR left a comment •

edited

mertcanaltin commented Dec 7, 2022 •

edited