test_runner: bail #48919

marco-ippolito · 2023-07-25T11:51:43Z

This is a first (ugly) implementation of the bail mechanism, I still wanted to open this draft pr to discuss it.
I'd love to hear your opinion 😃

nodejs-github-bot · 2023-07-25T11:51:48Z

Review requested:

@nodejs/test_runner

MoLow · 2023-07-25T12:37:05Z

this seems to run tests without reporting them after the bail

marco-ippolito · 2023-07-25T12:40:45Z

this seems to run tests without reporting them after the bail

Exactly, as I mentioned the problem at the moment is to identify when a test is failing, I think we should use ipc to increase a global counter and abort running test when bail is hit, I wanted to understand if there are other ways to achieve this.
Actually closing for the time being I want to rethink this before re publishing

cjihrig · 2023-07-25T13:29:55Z

I'd love to hear your opinion

I don't think this is the right approach. I don't think we need a counter or IPC. If you're adding a new --test-bail CLI flag, it will be forwarded to all of the worker processes. This will also allow --test-bail to be used without --test for individual test files.

Each individual file will bail out if it encounters an error, and send that information back to the main process. The main process would then be responsible for terminating all of the other worker processes.

marco-ippolito · 2023-07-25T13:53:17Z

I'd love to hear your opinion

I don't think this is the right approach. I don't think we need a counter or IPC. If you're adding a new --test-bail CLI flag, it will be forwarded to all of the worker processes. This will also allow --test-bail to be used without --test for individual test files.

Each individual file will bail out if it encounters an error, and send that information back to the main process. The main process would then be responsible for terminating all of the other worker processes.

The problem of this approach is that if you enter a value of 3, it will only bail if a single test file fails 3 times while I would expect to be global(?) i should check how other libraries handle it. If its per file it should be waay easier

cjihrig · 2023-07-25T14:01:47Z

Oh. I didn't read your docs. It seems strange to have a bail out limit of any value other than 1. That also complicates the implementation because you need to count failures in the test runner and each test file (to support with and without --test).

EDIT: Actually, we already have harness.counters.failed. If you search the codebase for the two call sites of countCompletedTest() you can detect when the bail out threshold is crossed.

marco-ippolito · 2023-07-26T04:27:30Z

I think I will go for the easy route, as Ive seen other libraries (eg vitest) when flagged with --bail exit at the first failure.

atlowChemi · 2023-07-26T05:45:41Z

@marco-ippolito A simple implementation for this solution exists in @MoLow's @reporter/bail package (https://github.com/MoLow/reporters/tree/main/packages/bail)

Perhaps we could add a new reporter and that keeps track of the amount of failed tests (by the amount of test:fail events etc)
BTW, I think having --test-bail default to 1 when flag was passed would be more intuitive (if not passed do not bail [bail 0] if passed with no value bail on first failure [bail 1])

MoLow · 2023-07-26T05:58:18Z

as @cjihrig has written we already keep track of that, even when using --test under root.harness

marco-ippolito · 2023-07-26T11:41:53Z

I was thinking for a first implementation I'd leave the flag boolean, and exit at first failure, I could use 0 and 1 but doesn't make much difference at the moment

atlowChemi

I think we should allow setting bail in the options for coded version as well
https://github.com/atlowChemi/node/blob/4b3d964a8c01c4b541fc85d3a419b58524806f04/lib/internal/test_runner/runner.js#L428

cjihrig · 2023-07-26T13:07:59Z

I was thinking for a first implementation

If you're planning to iterate and change the design, you'll need to make this an experimental feature. The test runner is stable now, so we won't be breaking things without a good reason.

marco-ippolito · 2023-07-26T13:29:52Z

I think it depends, if we will ever want to implement bailout as integer value bigger than 1, probably not.

ShogunPanda · 2023-07-27T14:36:38Z

IMHO numeric bail will never be needed. Boolean as you already coded is the way to go.

atlowChemi

I think there should also be an additional test validating that tests fail with the expected error type

lib/internal/test_runner/test.js

doc/api/test.md

doc/api/cli.md

doc/api/errors.md

doc/api/test.md

src/node_options.cc

cjihrig · 2023-08-01T22:02:26Z

It would be nice if we could better leverage the existing cancellation logic.

This reverts commit c4e3654.

This reverts commit c400ed1.

MoLow · 2023-08-30T04:49:57Z

lib/internal/test_runner/reporter/tap.js

@@ -32,6 +33,8 @@ async function * tapReporter(source) {
  for await (const { type, data } of source) {
    switch (type) {
      case 'test:fail': {
+        // If test bailed out do not report


so a test can run without us reporting it? I am not sure this is good

The test has error kBailedOut because has been canceled, I dont want to see canceled tests in the reporter because they bailedout

MoLow · 2023-08-30T05:00:45Z

I have some concerns about this implementation which seems indeterministic and unpredictable:

From the implementation it seems that in some cases (especially when using concurrency:true) bailing a test file will cause some tests to run but skip reporting. That isn't the actual motivation of bailing - after a first failure tests should stop running but we should'nt just ignore those that have started
When running multiple files (i.e node --test **.test.js) I haven't seen any mechanism that cancels running other files, so only the file with a failure would bail but others will continue running and just not be reported

marco-ippolito · 2023-08-30T07:12:32Z

Tests that have already started are canceled by this.root.postRun().
I will add a test with a folder to show the behavior when running with multiple test.
If a test bails, othed tests that have started should be canceled and not be reported

mcollina

lgtm

cjihrig

Blocking this since it has an approval now.

I share @MoLow's concerns in #48919 (comment). Looking at, for example, test/fixtures/test-runner/output/bail-error.snapshot the output looks wrong outside of the context of this PR.

marco-ippolito · 2023-08-30T23:03:41Z

Sorry maybe this is harder than I thought, I can't dedicate more time on it, feel free to take over.

cjihrig · 2023-09-01T13:31:35Z

@nodejs/test_runner I think we should decide if we want to support bailing out, and if so, what the experience looks like?

My opinion is that we should support it. This is how I would implement it, but I'm open to input from others:

If bail out mode is enabled, we emit a test:bail event.
Once a failure has occurred, we cancel any remaining tests.
Probably the step that needs the most discussion: In the built in reporters, we simply exit the process on the test:bail event. We should ensure that the failing test is reported, and the TAP reporter should print Bail out!. This way we don't need to output a bunch of cancelled tests or worry about the test summary, code coverage, etc. If custom reporters choose not to exit the process, it should be fine because we have made sure the remaining tests are cancelled.

Thoughts?

rluvaton · 2023-09-01T14:17:14Z

@cjihrig Maybe open a new issue and we can discuss it there?

atlowChemi · 2023-09-01T15:31:43Z

@cjihrig I generally agree, but I think we also need to decide on the expected behavior of the combination of bail with watch.
I would expect them to work together in the way that it would bail the current "iteration", and when any watched file changed start again.
WDYT?

cjihrig · 2023-09-01T16:21:00Z

@atlowChemi I agree. Off the top of my head I'm not 100% sure what the best way to implement that would be, but I agree with that behavior.

MoLow · 2023-09-03T05:55:31Z

Once a failure has occurred, we cancel any remaining tests.

doing that within a single file is trivial, but canceling running files might not always work if we use signals and they are caught by userland code

I generally agree, but I think we also need to decide on the expected behavior of the combination of bail with watch.

I think in that case opon test failure we should cancel all tests, empty the tests queue as, emit test:bail and continue from that point as usual (waiting for changes in files to enqueue more tests)

atlowChemi · 2023-09-03T12:12:33Z

doing that within a single file is trivial, but canceling running files might not always work if we use signals and they are caught by userland code

What do you mean "caught by userland code"? Are you referring to userland calling stopImmediatePropagation?

MoLow · 2023-09-03T12:24:27Z

What do you mean "caught by userland code"? Are you referring to userland calling stopImmediatePropagation?

I mean listening to SIGINT would prevent us from safely relying on it for graceful cancelation of test files, but we can always use SIGKILL instead

atlowChemi · 2023-09-03T14:28:51Z

Oh different kind of signals, got you now, thanks 👍🏽

marco-ippolito added the wip Issues and PRs that are still a work in progress. label Jul 25, 2023

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 Jul 25, 2023

marco-ippolito added the test_runner Issues and PRs related to the test runner subsystem. label Jul 25, 2023

marco-ippolito changed the title ~~test_runner: test runner bail~~ test_runner: bail Jul 25, 2023

marco-ippolito closed this Jul 25, 2023

marco-ippolito reopened this Jul 26, 2023

marco-ippolito force-pushed the feat/bail-test branch 2 times, most recently from 0d6a381 to 41aa463 Compare July 26, 2023 11:39

atlowChemi reviewed Jul 26, 2023

View reviewed changes

marco-ippolito force-pushed the feat/bail-test branch from 10098ba to fe9b46f Compare August 1, 2023 08:11

This comment was marked as resolved.

Sign in to view

marco-ippolito marked this pull request as ready for review August 1, 2023 09:50

marco-ippolito removed the wip Issues and PRs that are still a work in progress. label Aug 1, 2023

atlowChemi reviewed Aug 1, 2023

View reviewed changes

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

doc/api/test.md Outdated Show resolved Hide resolved

cjihrig reviewed Aug 1, 2023

View reviewed changes

doc/api/cli.md Show resolved Hide resolved

doc/api/cli.md Outdated Show resolved Hide resolved

doc/api/errors.md Outdated Show resolved Hide resolved

doc/api/test.md Outdated Show resolved Hide resolved

src/node_options.cc Outdated Show resolved Hide resolved

marco-ippolito added 9 commits August 29, 2023 12:42

fix: man

0074fa2

fix: must not call

4291a2a

fix: fix tests

0d8ec96

chore: mark as experimental

e91c680

feat: event test:bail

6e012a1

fix: more bailout

f9d1765

Revert "fix: more bailout"

c508639

This reverts commit c4e3654.

Revert "feat: event test:bail"

5b69804

This reverts commit c400ed1.

fix: removed tap bail and added snapshots

6678a3b

marco-ippolito force-pushed the feat/bail-test branch from 4535de4 to 6678a3b Compare August 29, 2023 10:43

github-actions bot mentioned this pull request Aug 30, 2023

CI Reliability 2023-08-30 nodejs/reliability#650

Open

16 tasks

MoLow reviewed Aug 30, 2023

View reviewed changes

mcollina approved these changes Aug 30, 2023

View reviewed changes

cjihrig requested changes Aug 30, 2023

View reviewed changes

marco-ippolito closed this Aug 30, 2023

This was referenced Aug 31, 2023

CI Reliability 2023-08-31 nodejs/reliability#651

Open

CI Reliability 2023-09-01 nodejs/reliability#652

Open

github-actions bot mentioned this pull request Sep 2, 2023

CI Reliability 2023-09-02 nodejs/reliability#653

Open

22 tasks

marco-ippolito mentioned this pull request Apr 29, 2024

Allow bailing early when test fails #52717

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test_runner: bail #48919

test_runner: bail #48919

marco-ippolito commented Jul 25, 2023 •

edited

Loading

nodejs-github-bot commented Jul 25, 2023

MoLow commented Jul 25, 2023

marco-ippolito commented Jul 25, 2023 •

edited

Loading

cjihrig commented Jul 25, 2023

marco-ippolito commented Jul 25, 2023 •

edited

Loading

cjihrig commented Jul 25, 2023 •

edited

Loading

marco-ippolito commented Jul 26, 2023

atlowChemi commented Jul 26, 2023

MoLow commented Jul 26, 2023

marco-ippolito commented Jul 26, 2023

atlowChemi left a comment

cjihrig commented Jul 26, 2023

marco-ippolito commented Jul 26, 2023 •

edited

Loading

ShogunPanda commented Jul 27, 2023

This comment was marked as resolved.

atlowChemi left a comment

cjihrig commented Aug 1, 2023

MoLow Aug 30, 2023

marco-ippolito Aug 30, 2023 •

edited

Loading

MoLow commented Aug 30, 2023

marco-ippolito commented Aug 30, 2023 •

edited

Loading

mcollina left a comment

cjihrig left a comment

marco-ippolito commented Aug 30, 2023

cjihrig commented Sep 1, 2023

rluvaton commented Sep 1, 2023

atlowChemi commented Sep 1, 2023

cjihrig commented Sep 1, 2023

MoLow commented Sep 3, 2023

atlowChemi commented Sep 3, 2023

MoLow commented Sep 3, 2023

atlowChemi commented Sep 3, 2023

test_runner: bail #48919

test_runner: bail #48919

Conversation

marco-ippolito commented Jul 25, 2023 • edited Loading

nodejs-github-bot commented Jul 25, 2023

MoLow commented Jul 25, 2023

marco-ippolito commented Jul 25, 2023 • edited Loading

cjihrig commented Jul 25, 2023

marco-ippolito commented Jul 25, 2023 • edited Loading

cjihrig commented Jul 25, 2023 • edited Loading

marco-ippolito commented Jul 26, 2023

atlowChemi commented Jul 26, 2023

MoLow commented Jul 26, 2023

marco-ippolito commented Jul 26, 2023

atlowChemi left a comment

Choose a reason for hiding this comment

cjihrig commented Jul 26, 2023

marco-ippolito commented Jul 26, 2023 • edited Loading

ShogunPanda commented Jul 27, 2023

This comment was marked as resolved.

atlowChemi left a comment

Choose a reason for hiding this comment

cjihrig commented Aug 1, 2023

MoLow Aug 30, 2023

Choose a reason for hiding this comment

marco-ippolito Aug 30, 2023 • edited Loading

Choose a reason for hiding this comment

MoLow commented Aug 30, 2023

marco-ippolito commented Aug 30, 2023 • edited Loading

mcollina left a comment

Choose a reason for hiding this comment

cjihrig left a comment

Choose a reason for hiding this comment

marco-ippolito commented Aug 30, 2023

cjihrig commented Sep 1, 2023

rluvaton commented Sep 1, 2023

atlowChemi commented Sep 1, 2023

cjihrig commented Sep 1, 2023

MoLow commented Sep 3, 2023

atlowChemi commented Sep 3, 2023

MoLow commented Sep 3, 2023

atlowChemi commented Sep 3, 2023

marco-ippolito commented Jul 25, 2023 •

edited

Loading

marco-ippolito commented Jul 25, 2023 •

edited

Loading

marco-ippolito commented Jul 25, 2023 •

edited

Loading

cjihrig commented Jul 25, 2023 •

edited

Loading

marco-ippolito commented Jul 26, 2023 •

edited

Loading

marco-ippolito Aug 30, 2023 •

edited

Loading

marco-ippolito commented Aug 30, 2023 •

edited

Loading