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

@MylesBorins v19.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#46440 Fixes: nodejs#45833 Refs: nodejs#45833 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

PR-URL: #46440 Backport-PR-URL: #46839 Fixes: #45833 Refs: #45833 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

PR-URL: #46440 Fixes: #45833 Refs: #45833 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

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 cjihrig and aduh95 January 31, 2023 15:07

cjihrig reviewed Feb 1, 2023

View reviewed changes

MoLow force-pushed the flatten-tap-output branch from 602e6bb to 0c377ea Compare 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 resolved Hide resolved

test/parallel/test-runner-cli.js Show resolved Hide resolved

lib/internal/test_runner/runner.js Show resolved Hide 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 bd1e3ba Compare 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 resolved Hide 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-actions bot 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-actions bot 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:main Feb 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 Indicate that the PR has an open backport. label Feb 25, 2023

juanarbol mentioned this pull request Mar 3, 2023

V18.15.0 proposal #46920

Merged

targos removed backport-open-v19.x backport-open-v18.x Indicate that the PR has an open backport. 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: #46440 Fixes: #45833 Refs: #45833 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

targos pushed a commit that referenced this pull request Mar 14, 2023

test_runner: flatten TAP output when running using --test

3a1a7fa

PR-URL: #46440 Fixes: #45833 Refs: #45833 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

targos mentioned this pull request Mar 14, 2023

v19.8.0 release proposal #47087

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

nodejs-github-bot commented Jan 31, 2023

cjihrig left a comment

cjihrig Feb 1, 2023

MoLow commented Feb 4, 2023

cjihrig left a comment

nodejs-github-bot commented Feb 16, 2023

nodejs-github-bot commented Feb 17, 2023

nodejs-github-bot commented Feb 17, 2023

nodejs-github-bot commented Feb 17, 2023

cjihrig left a comment

nodejs-github-bot commented Feb 18, 2023

MylesBorins commented Feb 18, 2023

MoLow commented Feb 18, 2023

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

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

Conversation

MoLow commented Jan 31, 2023 • edited Loading

nodejs-github-bot commented Jan 31, 2023

cjihrig left a comment

Choose a reason for hiding this comment

cjihrig Feb 1, 2023

Choose a reason for hiding this comment

MoLow commented Feb 4, 2023

cjihrig left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Feb 16, 2023

nodejs-github-bot commented Feb 17, 2023

nodejs-github-bot commented Feb 17, 2023

nodejs-github-bot commented Feb 17, 2023

cjihrig left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Feb 18, 2023

MylesBorins commented Feb 18, 2023

MoLow commented Feb 18, 2023

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