Skip to content

Conversation

@pulkit-30
Copy link
Contributor

@pulkit-30pulkit-30 commented Jan 23, 2023

fix: #45836

code

const test = require('node:test'); test('escaped description \\ # \\#\\ \n \t \f \v \b \r'); 
  • tap escaping without --test
TAP version 13 # Subtest: escaped description \\ \# \\\#\\ \\n \\t \\f \\v \\b \\r ok 1 - escaped description \\ \# \\\#\\ \\n \\t \\f \\v \\b \\r --- duration_ms: 3.569875 ... 1..1 # tests 1 # pass 1 # fail 0 # cancelled 0 # skipped 0 # todo 0 # duration_ms 6.989541 
  • tap escaping with --test
TAP version 13 # Subtest: /Users/pulkitgupta/Desktop/node/test.js # Subtest: escaped description \\ \# \\\#\\ \\n \\t \\f \\v \\b \\r ok 1 - escaped description \\ \# \\\#\\ \\n \\t \\f \\v \\b \\r --- duration_ms: 2.582292 ... 1..1 ok 1 - /Users/pulkitgupta/Desktop/node/test.js --- duration_ms: 49.498875 ... 1..1 # tests 1 # pass 1 # fail 0 # cancelled 0 # skipped 0 # todo 0 # duration_ms 50.930334 

Also modify test output's [test/message/test_runner_output.out , test/message/test_runner_output_cli.out]

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-botnodejs-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 Jan 23, 2023
Copy link
Member

@benjamingrbenjamingr left a comment

Choose a reason for hiding this comment

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

Maybe add a test that running with and without --test produces the same output?

@pulkit-30
Copy link
ContributorAuthor

Hey @benjamingr,
Thanks for the review,
Already there are tests for this (test_runner_output.js, test_runner_output_cli.js), that runs with and without --test flag,
I have modified output for these tests to ensure consistent/same output with and without --test flag.

Please let me know if more tests are required for the same;

@MoLowMoLow requested a review from benjamingrJanuary 26, 2023 07:42
@MoLowMoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 26, 2023
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 26, 2023
@benjamingrbenjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 28, 2023
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 28, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@MoLowMoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 28, 2023
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 28, 2023
@nodejs-github-botnodejs-github-bot merged commit 2f38c74 into nodejs:mainJan 28, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 2f38c74

ruyadorno pushed a commit that referenced this pull request Feb 1, 2023
PR-URL: #46311Fixes: #45836 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
@ruyadornoruyadorno mentioned this pull request Feb 1, 2023
MoLow pushed a commit to MoLow/node-core-test that referenced this pull request Feb 7, 2023
PR-URL: nodejs/node#46311Fixes: 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 pull request Feb 8, 2023
PR-URL: nodejs/node#46311Fixes: 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 pull request Feb 25, 2023
PR-URL: nodejs#46311Fixes: nodejs#45836 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
MoLow pushed a commit to MoLow/node that referenced this pull request Feb 25, 2023
PR-URL: nodejs#46311Fixes: nodejs#45836 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
MoLow pushed a commit to MoLow/node that referenced this pull request Feb 25, 2023
PR-URL: nodejs#46311Fixes: nodejs#45836 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
juanarbol pushed a commit that referenced this pull request Mar 3, 2023
PR-URL: #46311 Backport-PR-URL: #46839Fixes: #45836 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
@juanarboljuanarbol mentioned this pull request Mar 3, 2023
juanarbol pushed a commit that referenced this pull request Mar 5, 2023
PR-URL: #46311 Backport-PR-URL: #46839Fixes: #45836 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ciPRs that need a full CI run.test_runnerIssues and PRs related to the test runner subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tap escaping not consistent with and without --test

4 participants

@pulkit-30@nodejs-github-bot@benjamingr@MoLow