GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — test_runner: detect only tests when isolation is off by cjihrig · Pull Request #54832 · nodejs/node

Conversation

@cjihrig

@cjihrig cjihrig commented Sep 7, 2024

Copy link
Copy Markdown
Contributor
test_runner: apply filtering when tests begin

This commit updates the way filtering is applied to tests and
suites. After this change, filters are applied just before the
test/suite is started. The results are the same, but this allows
us to eventually move away from the --test-only flag except
when process level isolation is used.

test_runner: detect only tests when isolation is off

This commit updates the way the test runner processes 'only'
tests when process-based test isolation is disabled. The
--test-only flag is no longer necessary in this scenario. The
test runner will automatically detect 'only' tests and apply the
appropriate filtering.

This is not a breaking change because disabling test isolation is currently an experimental feature.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tonivj5, iambumblehead, and revir reacted with heart emoji avivkeller, bricss, tonivj5, and revir reacted with rocket emoji
All reactions
  • ❤️ 3 reactions
  • 🚀 4 reactions
@cjihrig
cjihrig requested review from MoLow, atlowChemi and benjamingr September 7, 2024 15:21
@nodejs-github-bot

nodejs-github-bot commented Sep 7, 2024

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/test_runner
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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 Sep 7, 2024
@cjihrig cjihrig added the commit-queue-rebase PRs the Commit Queue should land as multiple self-contained commits. label Sep 7, 2024
@avivkeller avivkeller added the experimental Issues and PRs related to experimental features. label Sep 7, 2024
avivkeller
avivkeller reviewed Sep 7, 2024
View reviewed changes
Comment thread doc/api/cli.md Outdated

@avivkeller avivkeller Sep 7, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

IMO this should link to the test isolation docs

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tonivj5 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread doc/api/test.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread doc/api/test.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
@codecov

codecov Bot commented Sep 7, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.07%. Comparing base (9f5977f) to head (f5f67ae).
Report is 139 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #54832      +/-   ##
==========================================
+ Coverage   88.04%   88.07%   +0.02%     
==========================================
  Files         651      651              
  Lines      183386   183409      +23     
  Branches    35820    35826       +6     
==========================================
+ Hits       161471   161529      +58     
+ Misses      15157    15142      -15     
+ Partials     6758     6738      -20
Files with missing lines Coverage Δ
lib/internal/test_runner/harness.js 93.37% <100.00%> (+0.11%) ⬆️
lib/internal/test_runner/test.js 96.97% <100.00%> (+0.03%) ⬆️

... and 23 files with indirect coverage changes
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
atlowChemi
atlowChemi reviewed Sep 7, 2024
View reviewed changes
Comment thread doc/api/test.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
atlowChemi
atlowChemi approved these changes Sep 7, 2024
View reviewed changes
benjamingr
benjamingr approved these changes Sep 7, 2024
View reviewed changes
jasnell
jasnell approved these changes Sep 7, 2024
View reviewed changes
@aduh95 aduh95 added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Sep 9, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 9, 2024
@nodejs-github-bot

nodejs-github-bot commented Sep 9, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/62221/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@targos

targos commented Sep 10, 2024

Copy link
Copy Markdown
Member

The test fails on Jenkins CI. Maybe an interaction with a commit that landed on main in the mean time?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cjihrig

cjihrig commented Sep 10, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

Yes, it looks like #54697 just landed recently and it uses fixtures that contain only tests. I'll update it.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cjihrig
cjihrig force-pushed the isolation-only branch from 1c54d7f to 03fb9e8 Compare September 10, 2024 12:47
@cjihrig cjihrig added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 10, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 10, 2024
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

nodejs-github-bot commented Sep 10, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/62257/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
MoLow
MoLow approved these changes Sep 10, 2024
View reviewed changes

@MoLow MoLow left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Nice!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot

nodejs-github-bot commented Sep 10, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/62286/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cjihrig cjihrig mentioned this pull request Sep 10, 2024
test_runner: detect only tests when --test is not used #54881
Merged
@cjihrig

cjihrig commented Sep 10, 2024

Copy link
Copy Markdown
Contributor Author

SmartOS was the only failure in the latest CI run. I believe it is only because the device is out of space:

Code coverage could not be enabled. Error: ENOSPC: no space left on device, mkdtemp '/tmp/node-coverage-XXXXXX'
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@richardlau

richardlau commented Sep 10, 2024

Copy link
Copy Markdown
Member

SmartOS was the only failure in the latest CI run. I believe it is only because the device is out of space:

Code coverage could not be enabled. Error: ENOSPC: no space left on device, mkdtemp '/tmp/node-coverage-XXXXXX'

Probably a recurrence of nodejs/build#3864.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Sep 11, 2024 •
edited by cjihrig
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/62344/ 🟡
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cjihrig cjihrig added commit-queue PRs queued for automated landing through the Commit Queue. and removed needs-ci PRs that need a full CI run. labels Sep 12, 2024
@nodejs-github-bot nodejs-github-bot added commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels Sep 12, 2024
@nodejs-github-bot

nodejs-github-bot commented Sep 12, 2024

Copy link
Copy Markdown
Collaborator
Commit Queue failed
- Loading data for nodejs/node/pull/54832
✔  Done loading data for nodejs/node/pull/54832
----------------------------------- PR info ------------------------------------
Title      test_runner: detect only tests when isolation is off (#54832)
Author     Colin Ihrig <[email protected]> (@cjihrig)
Branch     cjihrig:isolation-only -> nodejs:main
Labels     experimental, author ready, commit-queue-rebase, test_runner
Commits    2
 - test_runner: apply filtering when tests begin
 - test_runner: detect only tests when isolation is off
Committers 1
 - cjihrig <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/54832
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/54832
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sat, 07 Sep 2024 15:21:40 GMT
   ✔  Approvals: 4
   ✔  - Chemi Atlow (@atlowChemi): https://github.com/nodejs/node/pull/54832#pullrequestreview-2288101161
   ✔  - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/54832#pullrequestreview-2288149659
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/54832#pullrequestreview-2288420746
   ✔  - Moshe Atlow (@MoLow) (TSC): https://github.com/nodejs/node/pull/54832#pullrequestreview-2293610379
   ✘  Last GitHub CI failed
   ℹ  Last Full PR CI on 2024-09-11T23:50:32Z: https://ci.nodejs.org/job/node-test-pull-request/62344/
- Querying data for job/node-test-pull-request/62344/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/10830737962
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@avivkeller

avivkeller commented Sep 12, 2024

Copy link
Copy Markdown
Member

Last GitHub CI failed

🤔 The only failure was from Jenkins (https://ci.nodejs.org/job/node-test-commit-linux/nodes=alpine-latest-x64/60517/)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cjihrig

cjihrig commented Sep 12, 2024

Copy link
Copy Markdown
Contributor Author

I'm not sure what to do other than land this by hand. The last CI run was passing.
avivkeller reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cjihrig

cjihrig commented Sep 12, 2024

Copy link
Copy Markdown
Contributor Author

@targos or @richardlau can one of you confirm that this is OK to land with the failing ${STATUS_LABEL} check? It seems to be some sort of CI bug left over from a previous run.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@richardlau

richardlau commented Sep 12, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

@targos or @richardlau can one of you confirm that this is OK to land with the failing ${STATUS_LABEL} check? It seems to be some sort of CI bug left over from a previous run.

Yes it is okay to land. That's a rare issue when the job fails before it's able to set the STATUS_LABEL variable (which is one of the first things it tries to do).
cjihrig reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
cjihrig added 2 commits September 12, 2024 12:41
@cjihrig
test_runner: apply filtering when tests begin
e78fd8c 
This commit updates the way filtering is applied to tests and
suites. After this change, filters are applied just before the
test/suite is started. The results are the same, but this allows
us to eventually move away from the --test-only flag except
when process level isolation is used.

PR-URL: nodejs#54832
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
@cjihrig
test_runner: detect only tests when isolation is off
f5f67ae 
This commit updates the way the test runner processes 'only'
tests when process-based test isolation is disabled. The
--test-only flag is no longer necessary in this scenario. The
test runner will automatically detect 'only' tests and apply the
appropriate filtering.

PR-URL: nodejs#54832
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
@cjihrig
cjihrig force-pushed the isolation-only branch from 03fb9e8 to f5f67ae Compare September 12, 2024 16:57
@cjihrig
cjihrig merged commit f5f67ae into nodejs:main Sep 12, 2024
@cjihrig
cjihrig deleted the isolation-only branch September 12, 2024 16:57
@cjihrig

cjihrig commented Sep 12, 2024

Copy link
Copy Markdown
Contributor Author

Landed manually in e78fd8c and f5f67ae.
tonivj5 reacted with rocket emoji
All reactions
  • 🚀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cjihrig cjihrig mentioned this pull request Sep 15, 2024
having --test and --test-only flags increase prototyping time #47945
Closed
@RafaelGSS RafaelGSS mentioned this pull request Sep 16, 2024
v22.9.0 proposal #54966
Merged
RafaelGSS pushed a commit that referenced this pull request Sep 16, 2024
@cjihrig @RafaelGSS
test_runner: apply filtering when tests begin
e1cd87d 
This commit updates the way filtering is applied to tests and
suites. After this change, filters are applied just before the
test/suite is started. The results are the same, but this allows
us to eventually move away from the --test-only flag except
when process level isolation is used.

PR-URL: #54832
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Sep 16, 2024
@cjihrig @RafaelGSS
test_runner: detect only tests when isolation is off
0d36820 
This commit updates the way the test runner processes 'only'
tests when process-based test isolation is disabled. The
--test-only flag is no longer necessary in this scenario. The
test runner will automatically detect 'only' tests and apply the
appropriate filtering.

PR-URL: #54832
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Sep 17, 2024
@cjihrig @RafaelGSS
test_runner: apply filtering when tests begin
021f59b 
This commit updates the way filtering is applied to tests and
suites. After this change, filters are applied just before the
test/suite is started. The results are the same, but this allows
us to eventually move away from the --test-only flag except
when process level isolation is used.

PR-URL: #54832
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Sep 17, 2024
@cjihrig @RafaelGSS
test_runner: detect only tests when isolation is off
3396a49 
This commit updates the way the test runner processes 'only'
tests when process-based test isolation is disabled. The
--test-only flag is no longer necessary in this scenario. The
test runner will automatically detect 'only' tests and apply the
appropriate filtering.

PR-URL: #54832
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
@targos targos added the dont-land-on-v20.x label Sep 22, 2024
@pmarchini pmarchini mentioned this pull request Oct 8, 2024
test_runner: add cwd option to run #54705
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@avivkeller avivkeller avivkeller left review comments
@jasnell jasnell jasnell approved these changes
@benjamingr benjamingr benjamingr approved these changes
@MoLow MoLow MoLow approved these changes
@atlowChemi atlowChemi atlowChemi approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. commit-queue-rebase PRs the Commit Queue should land as multiple self-contained commits. experimental Issues and PRs related to experimental features. 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.

Uh oh!

There was an error while loading. Please reload this page.

10 participants

@cjihrig @nodejs-github-bot @targos @richardlau @avivkeller @jasnell @benjamingr @MoLow @atlowChemi @aduh95
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.