test: fix invalid output TAP if there newline in test name #45742

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.mjs
import { 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

lib/internal/test_runner/tap_stream.js

    StringPrototypeReplaceAll(input, '\\', '\\\\'), '#', '\\#'
-  );
+  ), '\n', '\\n');


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

test/parallel/test-runner-tap-parser-stream.js

@@ -4,6 +4,7 @@ const common = require('../common');
 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

lib/internal/test_runner/tap_stream.js

-  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?

return StringPrototypeReplaceAll(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:

let result = StringPrototypeReplaceAll(...); result = StringPrototypeReplaceAll(...); result = StringPrototypeReplaceAll(...); return result;

Got it 👍🏽
Thanks

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

lib/internal/test_runner/tap_stream.js

-  );
+  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: nodejs#45742 Fixes: nodejs#45396 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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

PR-URL: nodejs/node#45742 Fixes: 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 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 b3bc57f Compare 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-actions bot 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:main Dec 11, 2022

ErickWendel pushed a commit to ErickWendel/node that referenced this pull request Dec 12, 2022

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

53edee5

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

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: #45742 Fixes: #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: #45742 Fixes: #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 Dec 30, 2022

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

dc8c0b6

PR-URL: #45742 Fixes: #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: #45742 Fixes: #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: #45742 Fixes: #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 4, 2023

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

b27c0fe

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: fix invalid output TAP if there newline in test name #45742

test: fix invalid output TAP if there newline in test name #45742

pulkit-30 commented Dec 5, 2022 •

edited

aduh95 left a comment

cjihrig Dec 5, 2022

pulkit-30 commented Dec 7, 2022 •

edited

aduh95 commented Dec 7, 2022

cjihrig Dec 9, 2022

pulkit-30 Dec 10, 2022

cjihrig Dec 9, 2022

pulkit-30 Dec 10, 2022

cjihrig Dec 10, 2022

pulkit-30 Dec 10, 2022

cjihrig Dec 10, 2022

pulkit-30 Dec 10, 2022

nodejs-github-bot commented Dec 10, 2022

nodejs-github-bot commented Dec 11, 2022

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

test: fix invalid output TAP if there newline in test name #45742

test: fix invalid output TAP if there newline in test name #45742

Conversation

pulkit-30 commented Dec 5, 2022 • edited

aduh95 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pulkit-30 commented Dec 7, 2022 • edited

aduh95 commented Dec 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nodejs-github-bot commented Dec 10, 2022

nodejs-github-bot commented Dec 11, 2022

pulkit-30 commented Dec 5, 2022 •

edited

pulkit-30 commented Dec 7, 2022 •

edited