test: fix invalid output TAP if there newline in test name by pulkit-30 · Pull Request #45742 · nodejs/node

pulkit-30 · 2022-12-05T09:22:54Z

fix test outputs invalid TAP if there is a newline in the test name

Fixed Changes

Test Case:

// test.mjsimport{it}from'node:test'it('Hello \n world \t hello \b hello \v hello \f hello \r hello',()=>{});

Output

TAP version 13
# Subtest: Hello \n world \t hello \b hello \v hello \f hello \r hello
ok 1 - Hello \n world \t hello \b hello \v hello \f hello \r hello
---
duration_ms: 1.117041
...
1..1
# tests 1
# pass 1
# fail 0
# cancelled 0
# skipped 0
# todo 0
# duration_ms 6.629375

aduh95

This needs a test.

cjihrig · 2022-12-05T14:51:46Z

What about \r, \v, \b, and \f?

pulkit-30 · 2022-12-07T15:30:01Z

This needs a test.

Hey @aduh95
I'm not sure where to write tests for this. 😞
Could you please assist me with this?

aduh95 · 2022-12-07T18:10:47Z

I'm not sure where to write tests for this.

test/parallel/test-runner-tap-parser-stream.js seems an appropriate location.

cjihrig · 2022-12-09T04:16:23Z

 const assert = require('node:assert');
 const { TapParser } = require('internal/test_runner/tap_parser');
 const { TapChecker } = require('internal/test_runner/tap_checker');
+const { tapEscape } = require('internal/test_runner/tap_stream');


So, I don't think this file is the best place for testing this. This file is more about the TAP parser.
I would test this by doing the following:
Undo the changes in this file.
Remove the tapEscape export that this PR adds to lib/internal/test_runner/tap_stream.js.
Add a test to test/message/test_runner_output.js that uses \n, \r, etc (# and \ are already tested IIRC).
Update test/message/test_runner_output.out to reflect the changes in the test output.

updating this 👍🏽 🚀

cjihrig · 2022-12-09T04:24:19Z

- return StringPrototypeReplaceAll(
- StringPrototypeReplaceAll(input, '\\', '\\\\'), '#', '\\#'
- );
+ [{ escapeChar: '\\', tappedEscape: '\\\\' },


Even though the resulting code will not look as nice as this, I'm inclined to use a series of StringPrototypeReplaceAll() calls here instead.

@cjihrig like this... right?
returnStringPrototypeReplaceAll(StringPrototypeReplaceAll(StringPrototypeReplaceAll(StringPrototypeReplaceAll(StringPrototypeReplaceAll(StringPrototypeReplaceAll(StringPrototypeReplaceAll(StringPrototypeReplaceAll(input,'\\','\\\\'),'#','\\#'),'\b','\\b'),'\f','\\f'),'\t','\\t'),'\n','\\n'),'\r','\\r'),'\v','\\v');

You could do it that way. You could also do something like this to maintain more readability:
letresult=StringPrototypeReplaceAll(...);result=StringPrototypeReplaceAll(...);result=StringPrototypeReplaceAll(...);returnresult;

Got it 👍🏽
Thanks

cjihrig · 2022-12-10T16:07:59Z

- );
+ let result = StringPrototypeReplaceAll(input, '\\', '\\\\');
+ result = StringPrototypeReplaceAll(result, '#', '\\#');
+ result = StringPrototypeReplaceAll(result,'\b', '\\b');


Same change on the following lines. Other than that, I think this looks good.
Suggested change
result=StringPrototypeReplaceAll(result,'\b','\\b');
result=StringPrototypeReplaceAll(result,'\b','\\b');

Sorry, didn't notice that 😞
fixed 🚀

nodejs-github-bot · 2022-12-10T18:35:00Z

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

nodejs-github-bot · 2022-12-11T20:02:39Z

Landed in 22dc987

PR-URL: #45742Fixes: #45396 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

PR-URL: nodejs/node#45742Fixes: nodejs/node#45396 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> (cherry picked from commit 22dc987fde29734c5bcbb7c33da20d184ff61627)

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

aduh95 reviewed Dec 5, 2022
View reviewed changes

cjihrig reviewed Dec 5, 2022
View reviewed changes

pulkit-30 requested a review from cjihrig December 7, 2022 15:30

pulkit-30 added 3 commits December 8, 2022 19:00

test: fix invalid output TAP if there newline in test name
6ea386e

fix to replace some more escape chars
7ac8cba

added test for tapEscape function
b3bc57f

pulkit-30 force-pushed the fix-45396 branch from bbba3c7 to b3bc57fCompare December 8, 2022 13:31

cjihrig reviewed Dec 9, 2022
View reviewed changes

pulkit-30 added 2 commits December 10, 2022 21:19

moved test to test/message/test_runner_output.js file
e4ea715

used inclined way to escape chars
5b8e01e

cjihrig reviewed Dec 10, 2022
View reviewed changes

fix: lint errors
6a6f57e

MoLow approved these changes Dec 10, 2022
View reviewed changes

MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 10, 2022

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 10, 2022

cjihrig approved these changes Dec 11, 2022
View reviewed changes

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 11, 2022

nodejs-github-bot merged commit 22dc987 into nodejs:mainDec 11, 2022

targos pushed a commit that referenced this pull request Dec 12, 2022
test: fix invalid output TAP if there newline in test name
7eb315d
PR-URL: #45742Fixes: #45396 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

targos mentioned this pull request Dec 12, 2022
v19.3.0 release proposal #45831
Merged

targos pushed a commit that referenced this pull request Dec 13, 2022
test: fix invalid output TAP if there newline in test name
5c4475d
PR-URL: #45742Fixes: #45396 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

danielleadams mentioned this pull request Dec 30, 2022
v18.13.0 release proposal #46025
Merged

danielleadams pushed a commit that referenced this pull request Dec 30, 2022
test: fix invalid output TAP if there newline in test name
c867356
PR-URL: #45742Fixes: #45396 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

danielleadams pushed a commit that referenced this pull request Jan 3, 2023
test: fix invalid output TAP if there newline in test name
38fb2c1
PR-URL: #45742Fixes: #45396 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

danielleadams pushed a commit that referenced this pull request Jan 5, 2023
test: fix invalid output TAP if there newline in test name
b76f1ae
PR-URL: #45742Fixes: #45396 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

	result=StringPrototypeReplaceAll(result,'\b','\\b');
	result=StringPrototypeReplaceAll(result,'\b','\\b');

Uh oh!

Uh oh!

Conversation

pulkit-30 commented Dec 5, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduh95 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

aduh95 commented Dec 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Dec 10, 2022

Uh oh!

nodejs-github-bot commented Dec 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pulkit-30 commented Dec 5, 2022•
edited
Loading

pulkit-30 commented Dec 7, 2022•
edited
Loading