test_runner: detect only tests when isolation is off #54832

cjihrig · 2024-09-07T15:21:40Z

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.

nodejs-github-bot · 2024-09-07T15:21:44Z

Review requested:

@nodejs/test_runner

RedYetiDev · 2024-09-07T16:15:18Z

doc/api/cli.md

@@ -2336,7 +2336,7 @@ changes:
 -->

 Configures the test runner to only execute top level tests that have the `only`
-option set.
+option set. This flag is not necessary when test isolation is disabled.


IMO this should link to the test isolation docs

doc/api/test.md

codecov · 2024-09-07T16:41:30Z

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

doc/api/test.md

nodejs-github-bot · 2024-09-09T23:15:54Z

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

targos · 2024-09-10T06:23:51Z

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

cjihrig · 2024-09-10T12:08:57Z

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

nodejs-github-bot · 2024-09-10T14:36:28Z

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

MoLow

Nice!

nodejs-github-bot · 2024-09-10T20:40:45Z

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

cjihrig · 2024-09-10T21:49:06Z

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'

richardlau · 2024-09-10T23:12:08Z

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.

nodejs-github-bot · 2024-09-11T15:47:32Z

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

RedYetiDev · 2024-09-11T19:26:48Z

test/parallel/test-runner-no-isolation-hooks.mjs

@@ -30,15 +30,6 @@ const order = [
  'afterEach one: suite one - test',
  'afterEach two: suite one - test',

-  'beforeEach(): global',


Shouldn't these hooks be called?

Not for "test one" which is filtered out.

nodejs-github-bot · 2024-09-11T23:50:32Z

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

nodejs-github-bot · 2024-09-12T12:26:28Z

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

RedYetiDev · 2024-09-12T12:30:25Z

Last GitHub CI failed

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

cjihrig · 2024-09-12T12:32:47Z

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

cjihrig · 2024-09-12T15:07:30Z

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

richardlau · 2024-09-12T15:27:30Z

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

cjihrig · 2024-09-12T16:58:16Z

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>

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

cjihrig requested review from benjamingr, MoLow and atlowChemi 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

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

RedYetiDev reviewed Sep 7, 2024

View reviewed changes

atlowChemi reviewed Sep 7, 2024

View reviewed changes

doc/api/test.md Show resolved Hide resolved

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 pending requests for changes, 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

RedYetiDev reviewed Sep 11, 2024

View reviewed changes

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
21 checks passed

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 PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Sep 22, 2024

pmarchini mentioned this pull request Oct 8, 2024

test_runner: add cwd option to run #54705

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test_runner: detect only tests when isolation is off #54832

test_runner: detect only tests when isolation is off #54832

cjihrig commented Sep 7, 2024

nodejs-github-bot commented Sep 7, 2024

RedYetiDev Sep 7, 2024

codecov bot commented Sep 7, 2024 •

edited

Loading

nodejs-github-bot commented Sep 9, 2024

targos commented Sep 10, 2024

cjihrig commented Sep 10, 2024 •

edited

Loading

This comment was marked as outdated.

nodejs-github-bot commented Sep 10, 2024

MoLow left a comment

nodejs-github-bot commented Sep 10, 2024

cjihrig commented Sep 10, 2024

richardlau commented Sep 10, 2024

nodejs-github-bot commented Sep 11, 2024

RedYetiDev Sep 11, 2024

cjihrig Sep 11, 2024

nodejs-github-bot commented Sep 11, 2024 •

edited by cjihrig

Loading

nodejs-github-bot commented Sep 12, 2024

RedYetiDev commented Sep 12, 2024

cjihrig commented Sep 12, 2024

cjihrig commented Sep 12, 2024

richardlau commented Sep 12, 2024 •

edited

Loading

cjihrig commented Sep 12, 2024

test_runner: detect only tests when isolation is off #54832

test_runner: detect only tests when isolation is off #54832

Conversation

cjihrig commented Sep 7, 2024

test_runner: apply filtering when tests begin

test_runner: detect only tests when isolation is off

nodejs-github-bot commented Sep 7, 2024

RedYetiDev Sep 7, 2024

Choose a reason for hiding this comment

codecov bot commented Sep 7, 2024 • edited Loading

Codecov Report

nodejs-github-bot commented Sep 9, 2024

targos commented Sep 10, 2024

cjihrig commented Sep 10, 2024 • edited Loading

This comment was marked as outdated.

nodejs-github-bot commented Sep 10, 2024

MoLow left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Sep 10, 2024

cjihrig commented Sep 10, 2024

richardlau commented Sep 10, 2024

nodejs-github-bot commented Sep 11, 2024

RedYetiDev Sep 11, 2024

Choose a reason for hiding this comment

cjihrig Sep 11, 2024

Choose a reason for hiding this comment

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

nodejs-github-bot commented Sep 12, 2024

RedYetiDev commented Sep 12, 2024

cjihrig commented Sep 12, 2024

cjihrig commented Sep 12, 2024

richardlau commented Sep 12, 2024 • edited Loading

cjihrig commented Sep 12, 2024

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