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

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

doc/api/cli.md


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

avivkeller · 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 <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

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 · 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 <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

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