test_runner: add bail out #56490

pmarchini · 2025-01-06T19:15:47Z

Catching up with the last attempt (#48919), this is another try at introducing the bailout feature.
I'm opening this PR as a draft to discuss the implementation and because refactoring may be needed if this approach is well-received by the community.

Note: In some tests, I had to enforce a concurrency=1 setting because testing the bailout feature across multiple files concurrently proved to be extremely flaky.

nodejs-github-bot · 2025-01-06T19:15:54Z

Review requested:

@nodejs/test_runner

lib/internal/test_runner/runner.js

cjihrig · 2025-01-06T19:59:57Z

In some tests, I had to enforce a concurrency=1 setting because testing the bailout feature across multiple files concurrently proved to be extremely flaky.

Can you explain more about what was flaky? I'm guessing you mean the tests were at different points of execution when they received the bail out signal. I think the best way to work around this is to use test fixtures that never finish.

pmarchini · 2025-01-06T20:08:53Z

Can you explain more about what was flaky? I'm guessing you mean the tests were at different points of execution when they received the bail out signal. I think the best way to work around this is to use test fixtures that never finish.

Hey @cjihrig, you're guessing right!

Your proposed solution sounds good.

I think we should add a test as follows:

First file test: A test that fails after a long timeout (maybe 5-10 seconds) to allow other file test processes to be spawned correctly.
Second file test: A test with an infinite loop.

WDYT?

lib/internal/test_runner/runner.js

doc/api/test.md

cjihrig · 2025-01-06T20:16:20Z

doc/api/test.md

+By enabling this flag, the test runner will exit the test suite early
+when it encounters the first failing test, preventing
+the execution of subsequent tests.
+Already running tests will be canceled, and no further tests will be started.


I'm not sure if we need to get into this small of a detail, but technically there will be a window of time where new tests may be started before being cancelled. Maybe we can say something like "The test runner will cancel all remaining tests."

doc/api/test.md

cjihrig · 2025-01-06T20:18:04Z

doc/api/test.md

@@ -1483,6 +1512,11 @@ changes:
  does not have a name.
 * `options` {Object} Configuration options for the test. The following
  properties are supported:
+  * `bail` {boolean}


I don't think this should be an option to test(). Should it be part of run() instead?

Yes, my bad, the bail option it's part of run. I'll update the doc ASAP !

test/fixtures/test-runner/output/bail-spec.js

test/parallel/test-runner-run.mjs

cjihrig · 2025-01-06T20:38:28Z

test/parallel/test-runner-run.mjs

+    it('should only allow a boolean in options.bail', async () => {
+      [Symbol(), {}, [], () => { }, 0, 1, 0n, 1n, '', '1', Promise.resolve([])]
+        .forEach((bail) => assert.throws(() => run({ bail }), {
+          code: 'ERR_INVALID_ARG_TYPE'


Just out of curiosity, are we sure that these errors actually correspond to the use of bail? For example, it's possible that ERR_INVALID_ARG_TYPE or ERR_INVALID_ARG_VALUE are thrown by run() completely unrelated to the bail option.

I've written the test following a red-green cycle.
I'll take another look and I'll add a comment here 😁

Another option is to update the assertion to check the code and the message.

I agree, we should probably do the same for the other "validation" tests as well

test/parallel/test-runner-run.mjs

cjihrig · 2025-01-06T20:40:59Z

test/parallel/test-runner-run.mjs

+
+  // TODO(pmarchini): Bailout is not supported in watch mode yet but it should be.
+  // We should enable this test once it is supported.
+  it.todo('should handle the bail option with watch mode');


Should this be documented?

added a note into the documentation

cjihrig · 2025-01-06T20:52:06Z

I think we should add a test as follows:

First file test: A test that fails after a long timeout (maybe 5-10 seconds) to allow other file test processes to be spawned correctly.

Second file test: A test with an infinite loop.

There are all sorts of annoying edge cases to account for here, and it might be worth doing a survey of how the tap, mocha, and vitest runners handle bailing out when things are running in parallel. It's much more straightforward when only one thing is running. But, for example, if the very first test in the first process fails, do we bother spawning the other child processes at all? Or, in a bail out situation, how important is it to have an accurate summary at the end of the test run with correct counts for total tests, cancelled tests, etc.

pmarchini · 2025-01-12T10:38:57Z

@cjihrig I'm still checking different runners and it seems that mostly the behaviour is "non-standard".
Vitest returns, even after the bail out, the list of all the skipped test files.

Mocha stops the execution returning a partial result without reporting the full list of cancelled tests.

      const result = await runMochaAsync('options/parallel/test-*', [
        '--parallel',
        '--bail'
      ]);
      // we don't know _exactly_ how many tests will be skipped here
      // due to the --bail, but the number of tests completed should be
      // less than the total, which is 5.
      return expect(
        result.passing + result.pending + result.failing,
        'to be less than',
        5
      );

Checking tests like this one I have the impression that the testing of the feature itself follows a "best effort" approach in more than one tool.

While I'm still checking other examples I think that the most common use case for the bailout is to stop as soon as possible the execution in a CI/automation env.
In this scenario, IMHO, all that matters is the "fail fast" and "fail exit".
I'm not even sure that it would make any sense at all to provide a report after a bail and, if a report is being provided, then I prefer the idea of having an output that gives an exact trace of the actual run (so partial).
WDYT?

Regarding the tests I was thinking about:

sequential with single file
sequential isolation none
sequential isolation none with more than 1 test file
parallel with 2 test files ( 1 with some "loading-time" and 1 with infinite waiting loop )
parallel with 2+x test files with concurrency fixed to 2 -> this in order to cover the behaviour of one test file that should not even start its run
order of the hooks while in bail out -> to ensure the hooks are being run as supposed.
order of the hooks while in bail out -> multi file

Do you have any suggestions / other behaviours you think we should ensure?

codecov · 2025-01-12T23:11:14Z

Codecov Report

Attention: Patch coverage is 97.93814% with 2 lines in your changes missing coverage. Please review.

Project coverage is 89.20%. Comparing base (062ae6f) to head (db86232).
Report is 73 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/test_runner/runner.js	95.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #56490      +/-   ##
==========================================
+ Coverage   89.12%   89.20%   +0.08%     
==========================================
  Files         662      662              
  Lines      191555   191906     +351     
  Branches    36859    36948      +89     
==========================================
+ Hits       170722   171190     +468     
+ Misses      13694    13557     -137     
- Partials     7139     7159      +20

Files with missing lines	Coverage Δ
lib/internal/test_runner/harness.js	`92.83% <100.00%> (+0.16%)`	⬆️
lib/internal/test_runner/reporter/spec.js	`96.29% <100.00%> (+0.10%)`	⬆️
lib/internal/test_runner/reporter/tap.js	`95.43% <100.00%> (+0.06%)`	⬆️
lib/internal/test_runner/reporter/utils.js	`96.84% <100.00%> (+0.13%)`	⬆️
lib/internal/test_runner/test.js	`96.98% <100.00%> (+0.05%)`	⬆️
lib/internal/test_runner/tests_stream.js	`92.13% <100.00%> (+0.41%)`	⬆️
lib/internal/test_runner/utils.js	`58.79% <100.00%> (+2.58%)`	⬆️
src/node_options.cc	`87.96% <100.00%> (+0.03%)`	⬆️
src/node_options.h	`98.33% <ø> (+0.01%)`	⬆️
lib/internal/test_runner/runner.js	`90.30% <95.00%> (+0.79%)`	⬆️

... and 90 files with indirect coverage changes

cjihrig

Left some comments. I have some concerns:

The patch is pretty invasive for a feature that is not part of the default behavior. I'm not sure that it needs to be so invasive.
There seems to be a good bit of work being done when it is not necessary. Have you been able to do any performance comparisons with these changes.
The tests (thank you for adding so many tests!) seem to reliant on timers, which usually works fine locally, but is a recipe for flakiness in the CI.

cjihrig · 2025-01-13T15:35:30Z

lib/internal/test_runner/reporter/spec.js

@@ -57,6 +57,7 @@ class SpecReporter extends Transform {
  #handleEvent({ type, data }) {
    switch (type) {
      case 'test:fail':
+        if (data.details?.error?.failureType === kTestBailedOut) break;


I'm not a big fan of needing to check this for every failure, even if bail out mode is not enabled. Couldn't we break out in the test:bail event?

We could, but in that case, we would lose the "report' section.

Couldn't we break out in the test:bail event?

Specifically, how would you do that? Would you finalise the reporter, or propagate the abort up to the test runner root?

@pmarchini we need to also account for the fact these changes impact custom reporters

cjihrig · 2025-01-13T15:36:07Z

lib/internal/test_runner/reporter/tap.js

@@ -33,6 +33,7 @@ async function * tapReporter(source) {
  for await (const { type, data } of source) {
    switch (type) {
      case 'test:fail': {
+        if (data.details?.error?.failureType === lazyLoadTest().kTestBailedOut) break;


Same comment here.

Also, why is bailing out only supported in the spec and tap reporters? What about the dot reporter for example?

I haven't implemented all the reporters yet, as I'm still not convinced by the bail implementation itself.
I'll fix this before landing if we're able to reach an agreement on the implementation 🚀

cjihrig · 2025-01-13T15:40:28Z

lib/internal/test_runner/runner.js

@@ -663,6 +688,15 @@ function run(options = kEmptyObject) {

  validateStringArray(argv, 'options.argv');
  validateStringArray(execArgv, 'options.execArgv');
+  validateBoolean(bail, 'options.bail');


I think this should probably follow the same pattern as forceExit does around line 600 so that we don't need to perform all of this extra validation for no reason.

cjihrig · 2025-01-13T15:52:14Z

lib/internal/test_runner/test.js

-        'test did not finish before its parent and was cancelled',
-        kCancelledByParent,
-      ),
+    const cancelledError = this.root.bailed ? new ERR_TEST_FAILURE(


It seems wasteful to create this error unconditionally even though it might not be used.

cjihrig · 2025-01-13T15:53:26Z

lib/internal/test_runner/test.js

@@ -763,6 +770,13 @@ class Test extends AsyncResource {

    this.passed = false;
    this.error = err;
+
+    if (this.bail && !this.root.bailed) {


I'm not a big fan of adding another way to cancel tests when we already have logic for that.

Calling root.postRun was the only way I found to immediately propagate the "stop" to the entire test tree.
I'm currently trying to achieve the same using an AbortController, but I haven't found a viable solution yet.

cjihrig · 2025-01-13T15:58:49Z

test/fixtures/test-runner/bailout/parallel-concurrency/first.mjs

+
+  it('passing test with delay', async (t) => {
+    await new Promise((resolve) => setTimeout(resolve, 200));
+    t.assert.ok(true);


Is this needed?

cjihrig · 2025-01-13T16:00:14Z

test/fixtures/test-runner/bailout/parallel-concurrency/first.mjs

+describe('first parallel file', () => {
+
+  it('passing test with delay', async (t) => {
+    await new Promise((resolve) => setTimeout(resolve, 200));


We have the timers/promises module to avoid needing this pattern.

I also have concerns that if the test relies on this 200ms timeout, there is a good chance that it will become flaky in the CI. If there needs to be coordination between the files, it might be worth creating a file or something instead of relying on timers.

thanks for the advice, I'll try to follow your suggested approach! 🚀

cjihrig · 2025-01-13T16:00:39Z

test/fixtures/test-runner/bailout/parallel-concurrency/second.mjs

+
+describe('second parallel file', () => {
+  it('slow test that should be cancelled', async () => {
+    await new Promise((resolve) => setTimeout(resolve, 5000));


Same comment about using timers/promises. You can also just return the promise and the test runner will await it.

cjihrig · 2025-01-13T16:05:34Z

test/fixtures/test-runner/bailout/parallel-loading/infinite-loop.mjs

+  it('infinite loop that should be cancelled', async () => {
+    // This should be cancelled by the bail
+    while (true) {
+      await new Promise((resolve) => setTimeout(resolve, 100));


Same comment about timers/promises. I'll stop pointing it out, but it exists in some other places in this PR.

cjihrig · 2025-01-13T16:09:41Z

test/parallel/test-runner-bail.js

@@ -0,0 +1,174 @@
+'use strict';


There seems to be a lot of overlap with the changes to test/parallel/test-runner-run.mjs. That other file is getting pretty large, and --test already goes through run(), so I'd say eliminate any duplication that isn't needed.

pmarchini · 2025-01-13T16:41:56Z

Hey @cjihrig, thanks as always for your review.
I totally understand your concerns.

Step by step:

The patch is pretty invasive for a feature that is not part of the default behavior. I'm not sure that it needs to be so invasive.

Do you have any suggestions on how to reduce the footprint while better integrating it into the current implementation? I'm gaining more confidence in the codebase as I work on it, but I’m sure I might still be missing something important.

There seems to be a good bit of work being done when it is not necessary. Have you been able to do any performance comparisons with these changes?

Not yet, but I’ll make sure to do that as soon as possible!

The tests (thank you for adding so many tests!) seem too reliant on timers, which usually works fine locally, but is a recipe for flakiness in the CI.

I agree. I’ll try to use your suggested approach to coordinate the files without relying on timers or other flaky solutions.

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jan 6, 2025

pmarchini force-pushed the test_runner/bail-out branch from 89f64db to 55874e8 Compare January 6, 2025 19:18

pmarchini requested review from cjihrig, MoLow, marco-ippolito and jakecastelli January 6, 2025 19:18

atlowChemi reviewed Jan 6, 2025

View reviewed changes

lib/internal/test_runner/runner.js Outdated Show resolved Hide resolved

cjihrig reviewed Jan 6, 2025

View reviewed changes

test_runner: add bail out

f1c269c

pmarchini force-pushed the test_runner/bail-out branch from 55874e8 to f1c269c Compare January 10, 2025 17:36

pmarchini added 2 commits January 11, 2025 11:58

test_runner: replace process.kill with harness abort controller

1d68db4

test_runner: integrate abort signal handling in test tree creation

e691cd6

pmarchini added 6 commits January 12, 2025 19:32

test_runner: add bail out tests

d8efaec

test_runner: update bail-out documentation

e213dbf

test: lint fixtures

9514d89

test: fix assertion

a97c98b

test_runner: add bailout tests for long-running and multi-file cases

4d85df5

test: remove comments

39a21ba

pmarchini marked this pull request as ready for review January 12, 2025 21:51

pmarchini added 2 commits January 13, 2025 11:32

test_runner: remove unused test fixture references

0364d15

test_runner: remove temporary no-op for bailed tests

c3c7999

cjihrig reviewed Jan 13, 2025

View reviewed changes

pmarchini added 2 commits January 13, 2025 18:17

test: update fixture

eadba47

test_runner: avoid unnecessary error creation

db86232

test_runner: add bail out #56490

Are you sure you want to change the base?

test_runner: add bail out #56490

Conversation

pmarchini commented Jan 6, 2025

nodejs-github-bot commented Jan 6, 2025

cjihrig commented Jan 6, 2025

pmarchini commented Jan 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjihrig commented Jan 6, 2025

pmarchini commented Jan 12, 2025 • edited Loading

codecov bot commented Jan 12, 2025 • edited Loading

Codecov Report

cjihrig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmarchini commented Jan 13, 2025

pmarchini commented Jan 12, 2025 •

edited

Loading

codecov bot commented Jan 12, 2025 •

edited

Loading