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

Tap escaping not consistent with and without --test #45836

Closed
MoLow opened this issue Dec 13, 2022 · 6 comments · Fixed by #46311
Closed

Tap escaping not consistent with and without --test #45836

MoLow opened this issue Dec 13, 2022 · 6 comments · Fixed by #46311
Labels
good first issue Issues that are suitable for first-time contributors. test_runner Issues and PRs related to the test runner subsystem.

Comments

@MoLow
Copy link
Member

MoLow commented Dec 13, 2022

Version

v20.0.0-pre

Platform

Darwin Moshes-MBP.localdomain 21.1.0 Darwin Kernel Version 21.1.0: Wed Oct 13 17:33:01 PDT 2021; root:xnu-8019.41.5~1/RELEASE_ARM64_T6000 arm64

Subsystem

test_runner

What steps will reproduce the bug?

create test.js

const test = require('node:test');
test('escaped description \\ # \\#\\ \n \t \f \v \b \r');

How often does it reproduce? Is there a required condition?

No response

What is the expected behavior?

when running without --test:

node test.js

TAP version 13
# Subtest: escaped description \\ \# \\\#\\ \n \t \f \v \b \r
ok 1 - escaped description \\ \# \\\#\\ \n \t \f \v \b \r
  ---
  duration_ms: 1.318167
  ...
1..1
# tests 1
# pass 1
# fail 0
# cancelled 0
# skipped 0
# todo 0
# duration_ms 3.680459

What do you see instead?

when running with --test

node --test test.js

TAP version 13
# Subtest: /Users/moshe/repos/node/a.js
    # Subtest: escaped description \\ \# \\\#\\ n \\t f \\v b \\r
    ok 1 - escaped description \\ \# \\\#\\ n \\t f \\v b \\r
      ---
      duration_ms: 1.295459
      ...
    1..1
ok 1 - /Users/moshe/repos/node/a.js
  ---
  duration_ms: 80.91975
  ...
1..1
# tests 1
# pass 1
# fail 0
# cancelled 0
# skipped 0
# todo 0
# duration_ms 81.80225

Additional information

No response

@MoLow MoLow added good first issue Issues that are suitable for first-time contributors. test_runner Issues and PRs related to the test runner subsystem. labels Dec 13, 2022
@debadree25
Copy link
Member

@MoLow could you guide which relevant files I could look into, can take a stab at this

@MoLow
Copy link
Member Author

MoLow commented Dec 13, 2022

I am not sure where the exact issue is, but these can be good places to start the investigation

function tapEscape(input) {
let result = StringPrototypeReplaceAll(input, '\\', '\\\\');
result = StringPrototypeReplaceAll(result, '#', '\\#');
result = StringPrototypeReplaceAll(result, '\b', '\\b');
result = StringPrototypeReplaceAll(result, '\f', '\\f');
result = StringPrototypeReplaceAll(result, '\t', '\\t');
result = StringPrototypeReplaceAll(result, '\n', '\\n');
result = StringPrototypeReplaceAll(result, '\r', '\\r');
result = StringPrototypeReplaceAll(result, '\v', '\\v');
return result;
}

if (nextToken.kind !== TokenKind.ESCAPE) {
ArrayPrototypePush(literals, word);
}

@MoLow
Copy link
Member Author

MoLow commented Dec 13, 2022

also

#isEscapeSymbol(char) {

@debadree25
Copy link
Member

Thank you so much looking into it!

@7suyash7
Copy link
Contributor

$ node --test test.js
TAP version 13
# Subtest: /test/test.js
ok 1 - /test.js
  ---
  duration_ms: 0.036416797
  ...
1..1
# tests 1
# pass 1
# fail 0
# cancelled 0
# skipped 0
# todo 0
# duration_ms 0.079281594

Has this been fixed, here's the output which I get?

@cjihrig
Copy link
Contributor

cjihrig commented Dec 19, 2022

@manekinekko any ideas here?

nodejs-github-bot pushed a commit that referenced this issue Jan 28, 2023
PR-URL: #46311
Fixes: #45836
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
ruyadorno pushed a commit that referenced this issue Feb 1, 2023
PR-URL: #46311
Fixes: #45836
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
MoLow pushed a commit to MoLow/node-core-test that referenced this issue Feb 7, 2023
PR-URL: nodejs/node#46311
Fixes: nodejs/node#45836
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
(cherry picked from commit 2f38c74e263ed2e7f3b087efb9adee2442dd25c4)
MoLow pushed a commit to nodejs/node-core-test that referenced this issue Feb 8, 2023
PR-URL: nodejs/node#46311
Fixes: nodejs/node#45836
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
(cherry picked from commit 2f38c74e263ed2e7f3b087efb9adee2442dd25c4)
MoLow pushed a commit to MoLow/node that referenced this issue Feb 25, 2023
PR-URL: nodejs#46311
Fixes: nodejs#45836
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
MoLow pushed a commit to MoLow/node that referenced this issue Feb 25, 2023
PR-URL: nodejs#46311
Fixes: nodejs#45836
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
MoLow pushed a commit to MoLow/node that referenced this issue Feb 25, 2023
PR-URL: nodejs#46311
Fixes: nodejs#45836
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
juanarbol pushed a commit that referenced this issue Mar 3, 2023
PR-URL: #46311
Backport-PR-URL: #46839
Fixes: #45836
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
juanarbol pushed a commit that referenced this issue Mar 5, 2023
PR-URL: #46311
Backport-PR-URL: #46839
Fixes: #45836
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issues that are suitable for first-time contributors. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants