Skip to content

fix: Invalid test outputs cause of new line chars in test name fixed - #45552

Closed
scuro-dev wants to merge 1 commit into
nodejs:mainfrom
scuro-dev:invalid-tap-issue
Closed

fix: Invalid test outputs cause of new line chars in test name fixed#45552
scuro-dev wants to merge 1 commit into
nodejs:mainfrom
scuro-dev:invalid-tap-issue

Conversation

@scuro-dev

@scuro-dev scuro-dev commented Nov 20, 2022

Copy link
Copy Markdown

No description provided.

@nodejs-github-bot 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 Nov 20, 2022
@scuro-dev scuro-dev mentioned this pull request Nov 20, 2022
Closed
@MoLow

MoLow commented Nov 20, 2022

Copy link
Copy Markdown
Member

@sevkioruc thanks for your contribution! can you please add tests?

@scuro-dev

scuro-dev commented Nov 20, 2022 •
edited
Loading

Copy link
Copy Markdown
Author

@MoLow can i add tests to ./test/parralel/test-runner-run.mjs?

anonrig
anonrig reviewed Nov 21, 2022
View reviewed changes
Comment thread lib/internal/test_runner/tap_stream.js
return StringPrototypeReplaceAll(
StringPrototypeReplaceAll(input, '\\', '\\\\'), '#', '\\#'
);
StringPrototypeReplaceAll(

@anonrig anonrig Nov 21, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you fix the linting issues?

@Trott Trott Nov 22, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Running make lint-js-fix (or vcbuild lint-js-fix on Windows) should fix most of these lint issues automatically.

@anonrig

anonrig commented Nov 21, 2022

Copy link
Copy Markdown
Member

Commit message should start with test: prefix, since you are changing the test folder. Can you amend and force-push?

@MoLow

MoLow commented Nov 22, 2022

Copy link
Copy Markdown
Member

can i add tests to ./test/parralel/test-runner-run.mjs?

I think adding a message test will fit more this usecase

@ljharb

ljharb commented Nov 22, 2022

Copy link
Copy Markdown
Member

cc @nodejs/testing

@cjihrig

cjihrig commented Nov 27, 2022

Copy link
Copy Markdown
Contributor

@sevkioruc are you still working on this? If not, this is low hanging fruit that would be nice to get fixed.

@scuro-dev

scuro-dev commented Nov 28, 2022

Copy link
Copy Markdown
Author

Hey @cjihrig @MoLow , I am sorry i didn't work very well nowadays. Please unassing me this issue.

@cjihrig

cjihrig commented Dec 2, 2022

Copy link
Copy Markdown
Contributor

Thanks for getting back to us. I'll go ahead and close this PR if you aren't planning to pursue it.

@cjihrig cjihrig closed this Dec 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Trott Trott Trott left review comments
@anonrig anonrig anonrig left review comments

Assignees

No one assigned

Labels

needs-ci PRs that need a full CI run. 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.

7 participants

@scuro-dev @MoLow @anonrig @ljharb @cjihrig @Trott @nodejs-github-bot