test_runner: detect only tests when isolation is off by cjihrig · Pull Request #54832 · nodejs/node · GitHub

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.

Review requested:

@nodejs/test_runner

IMO this should link to the test isolation docs

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

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

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

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

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

Nice!

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

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'

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.

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

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 <cjihrig@gmail.com> (@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 <cjihrig@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/54832
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/54832
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
--------------------------------------------------------------------------------
   ℹ  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

Last GitHub CI failed

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

I'm not sure what to do other than land this by hand. The last CI run was passing.

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

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

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 <chemi@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

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 <chemi@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

Landed manually in e78fd8c and f5f67ae.

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 <chemi@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

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 <chemi@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

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 <chemi@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

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 <chemi@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

cjihrig requested review from MoLow, atlowChemi and benjamingr September 7, 2024 15:21

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 added the commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. label Sep 7, 2024

avivkeller added the experimental Issues and PRs related to experimental features. label Sep 7, 2024

avivkeller reviewed Sep 7, 2024

View reviewed changes

atlowChemi reviewed Sep 7, 2024

View reviewed changes

Comment thread doc/api/test.md Outdated

atlowChemi approved these changes Sep 7, 2024

View reviewed changes

benjamingr approved these changes Sep 7, 2024

View reviewed changes

jasnell approved these changes Sep 7, 2024

View reviewed changes

aduh95 added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 9, 2024

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 9, 2024

cjihrig force-pushed the isolation-only branch from 1c54d7f to 03fb9e8 Compare September 10, 2024 12:47

cjihrig added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 10, 2024

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 10, 2024

This comment was marked as outdated.

Sign in to view

MoLow approved these changes Sep 10, 2024

View reviewed changes

cjihrig mentioned this pull request Sep 10, 2024

test_runner: detect only tests when --test is not used #54881

Merged

cjihrig added commit-queue Add this label to land a pull request using GitHub Actions. and removed needs-ci PRs that need a full CI run. labels Sep 12, 2024

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Sep 12, 2024

cjihrig added 2 commits September 12, 2024 12:41

cjihrig force-pushed the isolation-only branch from 03fb9e8 to f5f67ae Compare September 12, 2024 16:57

cjihrig merged commit f5f67ae into nodejs:main Sep 12, 2024

cjihrig deleted the isolation-only branch September 12, 2024 16:57

cjihrig mentioned this pull request Sep 15, 2024

having --test and --test-only flags increase prototyping time #47945

Closed

RafaelGSS mentioned this pull request Sep 16, 2024

v22.9.0 proposal #54966

Merged

targos added the dont-land-on-v20.x label Sep 22, 2024

pmarchini mentioned this pull request Oct 8, 2024

test_runner: add cwd option to run #54705

Merged

test_runner: detect only tests when isolation is off by cjihrig · Pull Request #54832 · nodejs/node · GitHub

Uh oh!

Uh oh!

test_runner: detect only tests when isolation is off - #54832

test_runner: detect only tests when isolation is off#54832cjihrig merged 2 commits intonodejs:mainnodejs/node:mainfrom cjihrig:isolation-onlycjihrig/node:isolation-onlyCopy head branch name to clipboard

Conversation

cjihrig commented Sep 7, 2024

test_runner: apply filtering when tests begin

test_runner: detect only tests when isolation is off

Uh oh!

nodejs-github-bot commented Sep 7, 2024

Uh oh!

avivkeller Sep 7, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Sep 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 9, 2024

Uh oh!

targos commented Sep 10, 2024

Uh oh!

cjihrig commented Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

nodejs-github-bot commented Sep 10, 2024

Uh oh!

MoLow left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Sep 10, 2024

Uh oh!

cjihrig commented Sep 10, 2024

Uh oh!

richardlau commented Sep 10, 2024

Uh oh!

nodejs-github-bot commented Sep 11, 2024 • edited by cjihrig Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 12, 2024

Uh oh!

avivkeller commented Sep 12, 2024

Uh oh!

cjihrig commented Sep 12, 2024

Uh oh!

cjihrig commented Sep 12, 2024

Uh oh!

richardlau commented Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjihrig commented Sep 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

test_runner: detect only tests when isolation is off#54832
cjihrig merged 2 commits into
nodejs:mainfrom
cjihrig:isolation-only

codecov Bot commented Sep 7, 2024 •
edited
Loading

cjihrig commented Sep 10, 2024 •
edited
Loading

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

richardlau commented Sep 12, 2024 •
edited
Loading