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.

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

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

Merged
merged 2 commits into from
Feb 18, 2023

Conversation

MoLow
Copy link
Member

@MoLow MoLow commented Jan 31, 2023

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
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot 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 MoLow requested review from cjihrig and aduh95 January 31, 2023 15:07
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.

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 Show resolved Hide resolved
test/message/test_runner_output_cli.out Show resolved Hide resolved
lib/internal/test_runner/test.js Outdated Show resolved Hide resolved
Comment on lines 584 to 589
if (subtest.skipReporting || subtest.counted) {
return;
}
subtest.counted = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

I really dislike this block of code for a few reasons:

  1. skipReporting is defined on a subclass of Test. It seems like that logic should be contained to the FileTest class.
  2. counted changes the V8 shape of the Test objects. If this were going to be added, it should happen in the constructor, but...
  3. 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 Show resolved Hide resolved
lib/internal/test_runner/runner.js Outdated Show resolved Hide resolved
lib/internal/test_runner/runner.js Outdated Show resolved Hide resolved
lib/internal/test_runner/runner.js Outdated Show resolved Hide resolved
lib/internal/test_runner/runner.js Outdated Show resolved Hide resolved
@MoLow
Copy link
Member Author

MoLow commented Feb 4, 2023

@cjihrig I have pushed some changes addressing your feedback

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.

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

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
@MoLow MoLow added request-ci Add this label to start a Jenkins CI on a PR. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed request-ci Add this label to start a Jenkins CI on a PR. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Feb 14, 2023
@nodejs-github-bot
Copy link
Collaborator

@MoLow 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
@MoLow 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 github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 17, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@MoLow 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 nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Feb 17, 2023
@nodejs-github-bot
Copy link
Collaborator

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

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.

Fresh LGTM to appease the commit queue bot.

@MoLow 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 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 nodejs-github-bot merged commit 3354f89 into nodejs:main Feb 18, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 3354f89

@MoLow MoLow deleted the flatten-tap-output branch February 18, 2023 16:24
@MylesBorins
Copy link
Contributor

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

@MoLow
Copy link
Member Author

MoLow commented Feb 18, 2023

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

MoLow added a commit to MoLow/node that referenced this pull request Feb 18, 2023
PR-URL: nodejs#46440
Fixes: nodejs#45833
Refs: nodejs#45833
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MoLow added a commit to MoLow/node that referenced this pull request Feb 19, 2023
PR-URL: nodejs#46440
Fixes: nodejs#45833
Refs: nodejs#45833
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MoLow added a commit to MoLow/node that referenced this pull request Feb 25, 2023
PR-URL: nodejs#46440
Fixes: nodejs#45833
Refs: nodejs#45833
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MoLow added a commit to MoLow/node that referenced this pull request Feb 25, 2023
PR-URL: nodejs#46440
Fixes: nodejs#45833
Refs: nodejs#45833
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MoLow added a commit to MoLow/node that referenced this pull request Feb 25, 2023
PR-URL: nodejs#46440
Fixes: nodejs#45833
Refs: nodejs#45833
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@MoLow MoLow added the backport-open-v18.x Indicate that the PR has an open backport. label Feb 25, 2023
MoLow added a commit to MoLow/node that referenced this pull request Feb 25, 2023
PR-URL: nodejs#46440
Fixes: nodejs#45833
Refs: nodejs#45833
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
juanarbol pushed a commit that referenced this pull request Mar 3, 2023
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>
@juanarbol juanarbol mentioned this pull request Mar 3, 2023
juanarbol pushed a commit that referenced this pull request Mar 5, 2023
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>
@targos 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
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
PR-URL: #46440
Fixes: #45833
Refs: #45833
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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
Development

Successfully merging this pull request may close these issues.

node --test does not show skipped tests in TAP result
7 participants