test-runner: run tests as soon as find them by rluvaton · Pull Request #48584 · nodejs/node

rluvaton · 2023-06-28T13:31:01Z

currently find the tests files to run and then run them, this runs the tests as soon as finding them

Following our conversation in NodeTLV 2023:
Cc @MoLow @cjihrig

this is still WIP, just want the CI to run and general feedback

TODOs:

remove all the todos
wait for fs: add async glob and make it (async) iteratable #48594 to be merged

nodejs-github-bot · 2023-06-28T13:31:06Z

Review requested:

@nodejs/test_runner

aduh95 · 2023-06-28T13:38:01Z

lib/internal/fs/glob.js

+  async readdir(path) {
+    const cached = this.#readdirCache.get(path);
+    if (cached) {
+      return cached;
+    }
+    let val;
+    try {
+      val = await readdir(path, { __proto__: null, withFileTypes: true });
+    } catch {
+      val = [];
+    }
+    this.#readdirCache.set(path, val);


If the goal is to streamline the process of finding test files, shouldn't we be using opendir rather than readdir?

thanks, I need to check how I can insert it here, I did not change the logic...

+1 for opendir

rluvaton · 2023-06-28T16:42:37Z

test/parallel/test-runner-cli.js

  assert.strictEqual(child.status, 1);
  assert.strictEqual(child.signal, null);
-  assert.strictEqual(child.stdout.toString(), '');
+  assert.strictEqual(child.stdout.toString(), 'TAP version 13\n');


as we first start running and then check as we get the files asynchronously

this should not be outputed if no test files are fond. we should emit the start event only at the first test

lib/internal/test_runner/runner.js

ljharb · 2023-06-28T16:43:25Z

lib/internal/test_runner/runner.js

+    // TODO - find better way to check if promise
+    if (typeof testFiles.then === 'function') {


if it's not a promise, it's an array, right? could this check be inverted, so it can use IsArray, and then PromisePrototypeThen can be used unconditionally, and if it's not a promise, it'll throw?

(or if it's not an array and i'm just confused because it has .map, presumably there's another way to check whatever it is)

files can be array when provided by the user

it can be:

array: when provided by the user

stream: when we find the tests

promise: when we find the tests but we are in watch mode

ok, so IsArray works for array, and surely we have a way to brand check node's own streams?

do we need to? checked array and promise...

right, but there isn't a brand check for Promise, and this TODO comment references it.

Therefore, if you flip the conditions so you check the two other options, then you can just assume it's a promise.

gotcha, thanks

on another note, is there a way not to get a bazillion emails when the build cancels due to a new commit being pushed? it's really annoying!

I think the common interface should be anything that is async iterable. (arrays are, so are readable streams, so only watch mode needs to be adjusted to use an async iterable - that should not be hard since it uses an event emitter)

lib/internal/test_runner/runner.js

test/parallel/test-fs-glob.mjs

Co-authored-by: Jordan Harband <ljharb@gmail.com>

… into run-tests-as-soon-as-get-a-file # Conflicts: # lib/internal/test_runner/runner.js

rluvaton · 2023-06-28T17:16:02Z

lib/internal/test_runner/runner.js

+    // Overriding testFiles so we won't try to consume again the stream
+    testFiles = ArrayIsArray(testFiles) ? testFiles : testFiles.toArray();
+
+    // TODO - should not pass a promise


Any idea how can I avoid passing promise? as the tests need to have the filesWatcher, should I just mark the run function as async and await it normally?

testFiles should probably just be async iterable

rluvaton · 2023-06-28T17:45:52Z

lib/internal/test_runner/runner.js

+
+    if (isNodeStream(testFiles)) {
+      return testFiles
+        .map(runSingleFile, { __proto__: null, signal, concurrency: NumberMAX_SAFE_INTEGER })


used this concurrency to not wait for tests otherwise tests that not finish not allow us to run other tests, and this is also to keep the behavior of AllSettled...

will be changed to Infinity once the below PR is merged:

stream: allow infinite concurrency #48588

rluvaton · 2023-06-28T18:41:38Z

lib/internal/test_runner/harness.js

  const terminationHandler = async () => {
    await exitHandler();
-    process.exit();
+    process.exit(1);


changed to 1 as otherwise this test failed:

node/test/parallel/test-runner-exit-code.js

Line 25 in 384e1b5

assert.strictEqual(code, 1);

the exit code is equal to 0 instead of 1

weird it worked before...

I feel strongly like this should not be changed

I agree, changed it here until I fix the rest of the things

rluvaton · 2023-06-28T20:47:02Z

test/parallel/test-runner-cli.js

needed to change this file as the order of the tests file are not sorted anymore

test/parallel/test-runner-cli.js

MoLow

There is a lot going on in this PR.
I suggest to break out pieces to make it easier to review and test,
for example make globSync use Symbol.Iterator can be split out

MoLow · 2023-06-29T05:59:57Z

lib/internal/fs/glob.js

    }
  }
+
+  async *#addSubpatterns(path, pattern) {


we should try to avoid this code duplication, perhaps extracting logic to common methods will allow that

extracted in:

fs: add async glob and make it (async) iteratable #48594

MoLow · 2023-07-02T06:10:26Z

I know I am the one who suggested this optimization, but rethinking - this will make it much harder to implement feature requests such as #48619 or #48385, and I am not sure that it is really worth it

rluvaton · 2023-07-02T09:51:40Z

Why do you think it's harder to implement that way?

we can get all files in that case like we do in watch mode...

Also, sharding and randomization isn't the common case IMO

MoLow · 2023-07-02T12:12:03Z

I am Just saying there are many cases whre this optimization will not work with that it might be an overkill, since the gain isnt vert big anyway

cjihrig · 2023-08-11T15:45:55Z

Is this still being worked on or can it be closed?

rluvaton · 2023-08-12T19:30:56Z

Depending on what @MoLow thinks, I implemented sharding and this should not affect that It is just like watch - precomputing the file list (which will be the same as randomization tests)

MoLow · 2023-08-12T19:56:55Z

I am not blocking this - but it is probably a breaking change since tests wont run in the same order anymore

cjihrig · 2023-09-27T17:56:33Z

I'm going to close this for now since it doesn't appear to be progressing.

nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Jun 28, 2023

test_runner: run tests as soon as find them

1d80a2b

rluvaton force-pushed the run-tests-as-soon-as-get-a-file branch from 337a2ac to 1d80a2b Compare June 28, 2023 13:35

aduh95 reviewed Jun 28, 2023

View reviewed changes

test-runner: fix tests as files not run in consistent order...

20e7ad9

rluvaton commented Jun 28, 2023

View reviewed changes

ljharb reviewed Jun 28, 2023

View reviewed changes

rluvaton and others added 6 commits June 28, 2023 19:52

test: use hard-coded pattern

299cd6d

Update lib/internal/test_runner/runner.js

13f003e

Co-authored-by: Jordan Harband <ljharb@gmail.com>

Update lib/internal/test_runner/runner.js

a3f7575

Co-authored-by: Jordan Harband <ljharb@gmail.com>

test_runner: check node stream

00259ee

Merge remote-tracking branch 'origin/run-tests-as-soon-as-get-a-file'…

1164a4a

… into run-tests-as-soon-as-get-a-file # Conflicts: # lib/internal/test_runner/runner.js

test_runner: lint

afd0658

rluvaton commented Jun 28, 2023

View reviewed changes

test_runner: add comment

cebf726

rluvaton mentioned this pull request Jun 28, 2023

stream: allow infinite concurrency #48588

Open

test_runner: fix concurrent test

1d68c63

rluvaton commented Jun 28, 2023

View reviewed changes

rluvaton added 5 commits June 28, 2023 20:47

test_runner: format

0ea199f

test_runner: improve error message in test

98a9614

test_runner: fix exit code test

004e915

test_runner: revert termination handler process.exit(1)

0cb655f

test_runner: termination handler process.exit(1)

2062f53

rluvaton commented Jun 28, 2023

View reviewed changes

rluvaton added 3 commits June 28, 2023 21:43

test_runner: remove todo

517bdbc

test_runner: add signal support for glob

5d34f67

test_runner: simplify runner

4461789

rluvaton added 2 commits June 28, 2023 23:05

test_runner: remove comment

773f3cf

test_runner: remove todo

dd09109

rluvaton commented Jun 28, 2023

View reviewed changes

test/parallel/test-runner-cli.js

Copy link

Member Author

rluvaton Jun 28, 2023

Choose a reason for hiding this comment

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

needed to change this file as the order of the tests file are not sorted anymore

MoLow reviewed Jun 29, 2023

View reviewed changes

test/parallel/test-runner-cli.js Show resolved Hide resolved

MoLow reviewed Jun 29, 2023

View reviewed changes

MoLow mentioned this pull request Jun 29, 2023

test: add missing assertions to test-runner-cli #48593

Merged

rluvaton mentioned this pull request Jun 29, 2023

fs: add async glob and make it (async) iteratable #48594

Closed

cjihrig closed this Sep 27, 2023

		// TODO - find better way to check if promise
		if (typeof testFiles.then === 'function') {

Uh oh!

Conversation

rluvaton commented Jun 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODOs:

Uh oh!

nodejs-github-bot commented Jun 28, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MoLow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rluvaton Jun 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MoLow commented Jul 2, 2023

Uh oh!

rluvaton commented Jul 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MoLow commented Jul 2, 2023

Uh oh!

cjihrig commented Aug 11, 2023

Uh oh!

rluvaton commented Aug 12, 2023

Uh oh!

MoLow commented Aug 12, 2023

Uh oh!

cjihrig commented Sep 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

rluvaton commented Jun 28, 2023 •

edited

Loading

rluvaton Jun 29, 2023 •

edited

Loading

rluvaton commented Jul 2, 2023 •

edited

Loading