test_runner: flatten TAP output when running using `--test`#46440

MoLow · 2023-01-31T09:45:05Z

Refs: #45833 (comment)
Fixes: #45833

this removes the reporting of a test file and reports its children's tests directly
A few notable things/edge cases:

if a test file does not have child tests (e.g an empty file, a file that does not output TAP) - we will include it in the top-level output
if a test file has child tests, and none of them failed - but the exit code was non-zero - we will include it in the top-level output. an example of this is a test with extraneous async activity

also diff of test_runner_output_cli.out is 10X more readable when diff is set to ignore whitespaces

nodejs-github-bot · 2023-01-31T09:45:10Z

Review requested:

@nodejs/test_runner

cjihrig

Left some comments and have some concerns about the edge cases (which I left comments on), but looking nice.

test/message/test_runner_output_cli.out

test/parallel/test-runner-extraneous-async-activity.js

lib/internal/test_runner/test.js

cjihrig · 2023-02-01T17:55:32Z

lib/internal/test_runner/test.js

+if(subtest.skipReporting||subtest.counted){
+return;
+}
+subtest.counted=true;


I really dislike this block of code for a few reasons:
skipReporting is defined on a subclass of Test. It seems like that logic should be contained to the FileTest class.
counted changes the V8 shape of the Test objects. If this were going to be added, it should happen in the constructor, but...
The test runner is getting more and more of these checks added to determine if something already ran (via once() and other boolean checks). To me, this kind of implies that we don't actually know when code is executing, plus it slows things down. Shouldn't this only be called once per subtest?

lib/internal/test_runner/runner.js

MoLow · 2023-02-04T20:04:13Z

@cjihrig I have pushed some changes addressing your feedback

cjihrig

LGTM. Only one comment that I think we should address before landing (regarding testTimeoutFailures).

lib/internal/test_runner/test.js

test/parallel/test-runner-cli.js

lib/internal/test_runner/runner.js

nodejs-github-bot · 2023-02-16T23:13:59Z

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

lib/internal/test_runner/runner.js

nodejs-github-bot · 2023-02-17T06:10:04Z

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

nodejs-github-bot · 2023-02-17T11:14:48Z

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

nodejs-github-bot · 2023-02-17T15:59:24Z

Commit Queue failed

- Loading data for nodejs/node/pull/46440 ✔ Done loading data for nodejs/node/pull/46440 ----------------------------------- PR info ------------------------------------ Title test_runner: flatten TAP output when running using `--test` (#46440) Author Moshe Atlow (@MoLow) Branch MoLow:flatten-tap-output -> nodejs:main Labels author ready, needs-ci, dont-land-on-v14.x, commit-queue-squash, test_runner Commits 2 - test_runner: flatten TAP output when running using `--test` - CR Committers 1 - Moshe Atlow PR-URL: https://github.com/nodejs/node/pull/46440 Fixes: https://github.com/nodejs/node/issues/45833 Refs: https://github.com/nodejs/node/issues/45833 Reviewed-By: Colin Ihrig Reviewed-By: Benjamin Gruenbaum ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/46440 Fixes: https://github.com/nodejs/node/issues/45833 Refs: https://github.com/nodejs/node/issues/45833 Reviewed-By: Colin Ihrig Reviewed-By: Benjamin Gruenbaum -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last review: ⚠ - CR ℹ This PR was created on Tue, 31 Jan 2023 09:45:05 GMT ✔ Approvals: 2 ✔ - Colin Ihrig (@cjihrig) (TSC): https://github.com/nodejs/node/pull/46440#pullrequestreview-1294610234 ✔ - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/46440#pullrequestreview-1294768331 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2023-02-17T11:14:48Z: https://ci.nodejs.org/job/node-test-pull-request/49624/ - Querying data for job/node-test-pull-request/49624/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/4205387741

cjihrig

Fresh LGTM to appease the commit queue bot.

nodejs-github-bot · 2023-02-18T16:19:26Z

Landed in 3354f89

MylesBorins · 2023-02-18T21:08:49Z

This doesn't land cleanly on v19.x, could it please be backported

MoLow · 2023-02-18T21:38:52Z

@MylesBorinsv19.x-staging is missing a few commits (that land clean) that this depends on (61c65b0, 1118db7, 4c08c20, 2787e2d),
should I include them in the backport PR are will you push them into v19.x-staging?

PR-URL: nodejs#46440Fixes: nodejs#45833 Refs: nodejs#45833 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

PR-URL: #46440 Backport-PR-URL: #46839Fixes: #45833 Refs: #45833 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

PR-URL: #46440Fixes: #45833 Refs: #45833 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

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 Jan 31, 2023

MoLow requested review from aduh95 and cjihrig January 31, 2023 15:07

cjihrig reviewed Feb 1, 2023
View reviewed changes

MoLow force-pushed the flatten-tap-output branch from 602e6bb to 0c377eaCompare February 4, 2023 19:05

cjihrig mentioned this pull request Feb 10, 2023
Built-in test module only report ouput of the first top level describe#46478
Closed

cjihrig approved these changes Feb 11, 2023
View reviewed changes

lib/internal/test_runner/test.js Outdated Show resolvedHide resolved
test/parallel/test-runner-cli.jsShow resolvedHide resolved
lib/internal/test_runner/runner.jsShow resolvedHide resolved

benjamingr approved these changes Feb 12, 2023
View reviewed changes

test_runner: flatten TAP output when running using --test
bd1e3ba

MoLow force-pushed the flatten-tap-output branch from a64dbbc to bd1e3baCompare February 16, 2023 23:13

MoLow added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 16, 2023

aduh95 reviewed Feb 16, 2023
View reviewed changes

lib/internal/test_runner/runner.js Outdated Show resolvedHide resolved

CR
a1f6e9e

MoLow added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. request-ci Add this label to start a Jenkins CI on a PR. labels Feb 17, 2023

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 17, 2023

MoLow added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Feb 17, 2023

nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Feb 17, 2023

cjihrig approved these changes Feb 17, 2023
View reviewed changes

github-actionsbot mentioned this pull request Feb 18, 2023
CI Reliability 2023-02-18 nodejs/reliability#499
Open
32 tasks

MoLow added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Feb 18, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 18, 2023

nodejs-github-bot merged commit 3354f89 into nodejs:mainFeb 18, 2023

MoLow deleted the flatten-tap-output branch February 18, 2023 16:24

MylesBorins added the backport-requested-v19.x label Feb 18, 2023

MoLow added backport-open-v19.x and removed backport-requested-v19.x labels Feb 19, 2023

MoLow added the backport-open-v18.x label Feb 25, 2023

juanarbol mentioned this pull request Mar 3, 2023
V18.15.0 proposal #46920
Merged

targos removed backport-open-v19.x labels Mar 13, 2023

targos pushed a commit that referenced this pull request Mar 13, 2023
test_runner: flatten TAP output when running using --test
aa35ec7
PR-URL: #46440Fixes: #45833 Refs: #45833 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

targos pushed a commit that referenced this pull request Mar 14, 2023
test_runner: flatten TAP output when running using --test
3a1a7fa
PR-URL: #46440Fixes: #45833 Refs: #45833 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

targos mentioned this pull request Mar 14, 2023
v19.8.0 release proposal #47087
Merged

Uh oh!

test_runner: flatten TAP output when running using --test#46440

test_runner: flatten TAP output when running using --test#46440

Uh oh!

Conversation

MoLow commented Jan 31, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Jan 31, 2023

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cjihrigFeb 1, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MoLow commented Feb 4, 2023

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Feb 16, 2023

Uh oh!

Uh oh!

nodejs-github-bot commented Feb 17, 2023

Uh oh!

nodejs-github-bot commented Feb 17, 2023

Uh oh!

nodejs-github-bot commented Feb 17, 2023

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Feb 18, 2023

Uh oh!

MylesBorins commented Feb 18, 2023

Uh oh!

MoLow commented Feb 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

test_runner: flatten TAP output when running using `--test`#46440

test_runner: flatten TAP output when running using `--test`#46440

MoLow commented Jan 31, 2023•
edited
Loading