Skip to content

test_runner: add Subtest to tap protocol output - #43417

Merged
nodejs-github-bot merged 8 commits into
nodejs:mainfrom
MoLow:node-test-add-tap-sub-text
Jun 19, 2022
Merged

test_runner: add Subtest to tap protocol output#43417
nodejs-github-bot merged 8 commits into
nodejs:mainfrom
MoLow:node-test-add-tap-sub-text

Conversation

@MoLow

@MoLowMoLow commented Jun 14, 2022•
edited by VoltrexKeyva
Loading

Copy link
Copy Markdown
Member

just used tap-mocha-reporter to compare how output will look visually and saw it was missing

output for

test('level 1',asynct=>{awaitt.test('level 2.1',asynct=>{});awaitt.test('level 2.2',asynct=>{});awaitt.test('level 2.3',asynct=>{awaitt.test('level 2.3',asynct=>{});});});

before:
image
after
image

@MoLow

MoLow commented Jun 14, 2022

Copy link
Copy Markdown
MemberAuthor

cc @benjamingr

@nodejs-github-botnodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Jun 14, 2022
@MoLowMoLow changed the title node:test add Subtest to tap protocol outputtest_runner: add Subtest to tap protocol outputJun 14, 2022
@MoLow
MoLowforce-pushed the node-test-add-tap-sub-text branch from b24517e to 6ba42fcCompareJune 14, 2022 06:44
@benjamingr

benjamingr commented Jun 14, 2022

Copy link
Copy Markdown
Member

@MoLow the feature sounds good - the PR needs tests I think

cc @cjihrig @nodejs/test_runner

@MoLow

MoLow commented Jun 14, 2022

Copy link
Copy Markdown
MemberAuthor

@MoLow the feature sounds good - the PR needs tests I think

cc @cjihrig @nodejs/test_runner

@benjamingr there are currently no tests whatsoever for the TAP protocol output. would you test that using spawn and assertions on stdout?

@benjamingr

benjamingr commented Jun 14, 2022

Copy link
Copy Markdown
Member

@MoLow yes, probably - there are a lot of prior art examples of testing stdout against fixtures

benjamingr
benjamingr reviewed Jun 14, 2022
View reviewed changes
Comment threadtest/fixtures/test-runner/nested.test.js Outdated
@MoLow
MoLow requested a review from benjamingrJune 14, 2022 10:09
@benjamingrbenjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 14, 2022
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 14, 2022
@nodejs-github-bot

nodejs-github-bot commented Jun 14, 2022

Copy link
Copy Markdown
Collaborator

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

@MoLow
MoLowforce-pushed the node-test-add-tap-sub-text branch from 623767a to 4bca978CompareJune 14, 2022 14:11
cjihrig
cjihrig requested changes Jun 14, 2022
View reviewed changes

@cjihrigcjihrig left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Requesting changes since this already has an approval. The output doesn't look correct in some places.

Comment threadtest/message/test_runner_output.out Outdated
@MoLow
MoLow requested a review from cjihrigJune 14, 2022 17:21
@MoLow
MoLowforce-pushed the node-test-add-tap-sub-text branch from 5005c5c to d3c579dCompareJune 14, 2022 19:51
cjihrig
cjihrig reviewed Jun 14, 2022
View reviewed changes
Comment threadlib/internal/test_runner/tap_stream.js Outdated
Comment threadlib/internal/test_runner/test.js Outdated
@github-actionsgithub-actionsBot mentioned this pull request Jun 15, 2022
Open
17 tasks
@MoLow
MoLowforce-pushed the node-test-add-tap-sub-text branch from d3c579d to 1366dffCompareJune 15, 2022 07:30
@MoLow
MoLow requested a review from cjihrigJune 15, 2022 17:08
@github-actionsgithub-actionsBot mentioned this pull request Jun 17, 2022
Open
31 tasks
@benjamingrbenjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 17, 2022
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 17, 2022
@nodejs-github-bot

nodejs-github-bot commented Jun 17, 2022

Copy link
Copy Markdown
Collaborator

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

@github-actionsgithub-actionsBot mentioned this pull request Jun 18, 2022
Open
34 tasks
@F3n67uF3n67u mentioned this pull request Jun 18, 2022
Closed
@github-actionsgithub-actionsBot mentioned this pull request Jun 19, 2022
Open
30 tasks
@nodejs-github-bot

nodejs-github-bot commented Jun 19, 2022

Copy link
Copy Markdown
Collaborator

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

@benjamingrbenjamingr added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jun 19, 2022
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 19, 2022
@nodejs-github-bot
nodejs-github-bot merged commit 5fadc38 into nodejs:mainJun 19, 2022
@nodejs-github-bot

nodejs-github-bot commented Jun 19, 2022

Copy link
Copy Markdown
Collaborator

Landed in 5fadc38

@MoLow
MoLow deleted the node-test-add-tap-sub-text branch June 19, 2022 10:57
@github-actionsgithub-actionsBot mentioned this pull request Jun 20, 2022
Open
36 tasks
aduh95 pushed a commit to aduh95/node-core-test that referenced this pull request Jul 8, 2022
@MoLow@aduh95
test_runner: add Subtest to tap protocol output
28eb18d
PR-URL: nodejs/node#43417
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
aduh95 pushed a commit to aduh95/node-core-test that referenced this pull request Jul 9, 2022
@MoLow@aduh95
feat: add Subtest to tap protocol output
bd73127
PR-URL: nodejs/node#43417
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
aduh95 pushed a commit to aduh95/node-core-test that referenced this pull request Jul 9, 2022
@MoLow@aduh95
feat: add Subtest to tap protocol output
fc0256b
PR-URL: nodejs/node#43417
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit that referenced this pull request Jul 12, 2022
@MoLow@targos
test_runner: add Subtest to tap protocol output
dda64dd
PR-URL: #43417
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@targostargos mentioned this pull request Jul 12, 2022
Merged
@targos

targos commented Jul 18, 2022

Copy link
Copy Markdown
Member

Depends on #42658

@manekinekkomanekinekko mentioned this pull request Jul 29, 2022
Closed
@MoLowMoLow mentioned this pull request Jul 29, 2022
Closed
targos pushed a commit that referenced this pull request Jul 31, 2022
@MoLow@targos
test_runner: add Subtest to tap protocol output
1a42359
PR-URL: #43417
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@targostargos mentioned this pull request Aug 3, 2022
Merged
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@MoLow@guangwong
test_runner: add Subtest to tap protocol output
4510cf8
PR-URL: nodejs/node#43417
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes
@benjamingrbenjamingrbenjamingr approved these changes
@cjihrigcjihrigcjihrig approved these changes

Assignees

No one assigned

Labels

commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.needs-ciPRs that need a full CI run.test_runnerIssues 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

@MoLow@benjamingr@nodejs-github-bot@targos@jasnell@cjihrig@aduh95