test_runner: flatten TAP output when running using `--test` by MoLow · Pull Request #46440 · nodejs/node · GitHub

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

Review requested:

@nodejs/test_runner

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

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?

@cjihrig I have pushed some changes addressing your feedback

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

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

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

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

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

Fresh LGTM to appease the commit queue bot.

Landed in 3354f89

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

@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 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 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

Comment thread lib/internal/test_runner/test.js Outdated

Comment thread test/parallel/test-runner-cli.js Outdated

Comment thread lib/internal/test_runner/runner.js Outdated

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 outstanding review comments, and a CI started. label Feb 16, 2023

aduh95 reviewed Feb 16, 2023

View reviewed changes

Comment thread lib/internal/test_runner/runner.js Outdated

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 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: #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

test_runner: flatten TAP output when running using `--test` by MoLow · Pull Request #46440 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

test_runner: flatten TAP output when running using --test#46440nodejs-github-bot merged 2 commits intonodejs:mainnodejs/node:mainfrom MoLow:flatten-tap-outputMoLow/node:flatten-tap-outputCopy head branch name to clipboard

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!

cjihrig Feb 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
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
MoLow:flatten-tap-output

MoLow commented Jan 31, 2023 •
edited
Loading