Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test_runner: add Subtest to tap protocol output #43417

Merged
merged 8 commits into from
Jun 19, 2022
Merged

test_runner: add Subtest to tap protocol output #43417

merged 8 commits into from
Jun 19, 2022

Conversation

MoLow
Copy link
Member

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

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

output for

test('level 1', async t => {
    await t.test('level 2.1', async t => {});
    await t.test('level 2.2', async t => {});
    await t.test('level 2.3', async t => {
        await t.test('level 2.3', async t => {});
    });
});

before:
image
after
image

@MoLow
Copy link
Member Author

MoLow commented Jun 14, 2022

cc @benjamingr

@nodejs-github-bot nodejs-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
@MoLow MoLow changed the title node:test add Subtest to tap protocol output test_runner: add Subtest to tap protocol output Jun 14, 2022
@MoLow MoLow force-pushed the node-test-add-tap-sub-text branch from b24517e to 6ba42fc Compare June 14, 2022 06:44
@benjamingr
Copy link
Member

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

cc @cjihrig @nodejs/test_runner

@MoLow
Copy link
Member Author

MoLow commented Jun 14, 2022

@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
Copy link
Member

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

@MoLow MoLow requested a review from benjamingr June 14, 2022 10:09
@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 14, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 14, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@MoLow MoLow force-pushed the node-test-add-tap-sub-text branch from 623767a to 4bca978 Compare June 14, 2022 14:11
cjihrig
cjihrig requested changes Jun 14, 2022
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

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.

test/message/test_runner_output.out Outdated Show resolved Hide resolved
@MoLow MoLow requested a review from cjihrig June 14, 2022 17:21
@MoLow MoLow force-pushed the node-test-add-tap-sub-text branch from 5005c5c to d3c579d Compare June 14, 2022 19:51
cjihrig
cjihrig reviewed Jun 14, 2022
View reviewed changes
lib/internal/test_runner/tap_stream.js Outdated Show resolved Hide resolved
lib/internal/test_runner/test.js Outdated Show resolved Hide resolved
@github-actions github-actions bot mentioned this pull request Jun 15, 2022
Open
17 tasks
@MoLow MoLow force-pushed the node-test-add-tap-sub-text branch from d3c579d to 1366dff Compare June 15, 2022 07:30
@MoLow MoLow requested a review from cjihrig June 15, 2022 17:08
@github-actions github-actions bot mentioned this pull request Jun 17, 2022
Open
31 tasks
@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 17, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 17, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Jun 18, 2022
Open
34 tasks
@F3n67u F3n67u mentioned this pull request Jun 18, 2022
Open
@github-actions github-actions bot mentioned this pull request Jun 19, 2022
Open
30 tasks
@nodejs-github-bot
Copy link
Collaborator

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

@benjamingr benjamingr 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-bot nodejs-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:main Jun 19, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 5fadc38

@MoLow MoLow deleted the node-test-add-tap-sub-text branch June 19, 2022 10:57
@github-actions github-actions bot 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>
@targos targos mentioned this pull request Jul 12, 2022
Merged
@targos
Copy link
Member

targos commented Jul 18, 2022

Depends on #42658

@manekinekko manekinekko mentioned this pull request Jul 29, 2022
Closed
@MoLow MoLow 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>
@targos targos 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 free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@benjamingr benjamingr benjamingr approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
Labels
commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. 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
@MoLow @benjamingr @nodejs-github-bot @targos @jasnell @cjihrig @aduh95