GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — test: fix invalid output TAP if there newline in test name by pulkit-30 · Pull Request #45742 · nodejs/node

Conversation

pulkit-30 commented Dec 5, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

fixes: #45396

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss reacted with thumbs up emoji

All reactions

👍 1 reaction

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

aduh95 left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a test.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

cjihrig reviewed

Dec 5, 2022

View reviewed changes

Comment thread

lib/internal/test_runner/tap_stream.js Outdated

cjihrig Dec 5, 2022

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

MoLow reacted with thumbs up emoji

pulkit-30 reacted with eyes emoji

All reactions

👍 1 reaction
👀 1 reaction

pulkit-30 commented Dec 7, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor Author

This needs a test.

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

pulkit-30 requested a review from cjihrig

December 7, 2022 15:30

aduh95 commented Dec 7, 2022

Copy link

Copy Markdown

Contributor

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

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

pulkit-30 reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Comment thread

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

		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');

cjihrig Dec 9, 2022

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

pulkit-30 Dec 10, 2022

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updating this 👍🏽 🚀

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

lib/internal/test_runner/tap_stream.js Outdated

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

cjihrig Dec 9, 2022

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

pulkit-30 Dec 10, 2022

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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');

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

cjihrig Dec 10, 2022

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

pulkit-30 Dec 10, 2022

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it 👍🏽
Thanks

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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

Comment thread

lib/internal/test_runner/tap_stream.js Outdated

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

cjihrig Dec 10, 2022

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

pulkit-30 Dec 10, 2022

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, didn't notice that 😞
fixed 🚀

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

cjihrig reacted with rocket emoji

All reactions

🚀 1 reaction


          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. Only starts once the PR has an approving review. label

Dec 10, 2022

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Dec 10, 2022

nodejs-github-bot commented Dec 10, 2022

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

cjihrig approved these changes

Dec 11, 2022

View reviewed changes

cjihrig added author ready

PRs with CI started, the required approvals, and no outstanding review comments.

commit-queue

PRs queued for automated landing through the Commit Queue.

commit-queue-squash PRs the Commit Queue should land as one squashed commit. and removed needs-ci PRs that need a full CI run. labels

Dec 11, 2022

nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label

Dec 11, 2022

nodejs-github-bot merged commit 22dc987 into nodejs:main

Dec 11, 2022

nodejs-github-bot commented Dec 11, 2022

Copy link

Copy Markdown

Collaborator

Landed in 22dc987

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>

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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>

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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>

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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>

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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>

MoLow pushed a commit to MoLow/node-core-test that referenced this pull request

Feb 2, 2023


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

c0854ac

PR-URL: nodejs/node#45742
Fixes: nodejs/node#45396
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
(cherry picked from commit 22dc987fde29734c5bcbb7c33da20d184ff61627)

Reviewers

aduh95

aduh95 left review comments

cjihrig

cjihrig approved these changes

MoLow

MoLow approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready

PRs with CI started, the required approvals, and no outstanding review comments.

commit-queue-squash

PRs the Commit Queue should land as one squashed commit.

test_runner

Issues and PRs related to the test runner subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

Uh oh!

There was an error while loading. Please reload this page.

5 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

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

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pulkit-30 commented Dec 5, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

pulkit-30 commented Dec 7, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.