Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test_runner: bail #48919

Closed
wants to merge 25 commits into from
Closed

Conversation

marco-ippolito
Copy link
Member

@marco-ippolito marco-ippolito commented Jul 25, 2023

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
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@marco-ippolito marco-ippolito added the wip Issues and PRs that are still a work in progress. label Jul 25, 2023
@nodejs-github-bot 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 marco-ippolito added the test_runner Issues and PRs related to the test runner subsystem. label Jul 25, 2023
@marco-ippolito marco-ippolito changed the title test_runner: test runner bail test_runner: bail Jul 25, 2023
@MoLow
Copy link
Member

MoLow commented Jul 25, 2023

this seems to run tests without reporting them after the bail

@marco-ippolito
Copy link
Member Author

marco-ippolito commented Jul 25, 2023

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
Copy link
Contributor

cjihrig commented Jul 25, 2023

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
Copy link
Member Author

marco-ippolito commented Jul 25, 2023

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
Copy link
Contributor

cjihrig commented Jul 25, 2023

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
Copy link
Member Author

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
Copy link
Member

@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
Copy link
Member

MoLow commented Jul 26, 2023

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

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

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

Copy link
Member

@atlowChemi atlowChemi left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor

cjihrig commented Jul 26, 2023

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
Copy link
Member Author

marco-ippolito commented Jul 26, 2023

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

@ShogunPanda
Copy link
Contributor

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

@marco-ippolito

This comment was marked as resolved.

@marco-ippolito marco-ippolito marked this pull request as ready for review August 1, 2023 09:50
@marco-ippolito marco-ippolito removed the wip Issues and PRs that are still a work in progress. label Aug 1, 2023
Copy link
Member

@atlowChemi atlowChemi left a comment

Choose a reason for hiding this comment

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

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

lib/internal/test_runner/test.js Outdated Show resolved Hide resolved
doc/api/test.md Outdated Show resolved Hide resolved
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
@cjihrig
Copy link
Contributor

cjihrig commented Aug 1, 2023

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

@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@marco-ippolito marco-ippolito Aug 30, 2023

Choose a reason for hiding this comment

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

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

@MoLow
Copy link
Member

MoLow commented Aug 30, 2023

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
Copy link
Member Author

marco-ippolito commented Aug 30, 2023

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

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member Author

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

@cjihrig
Copy link
Contributor

cjihrig commented Sep 1, 2023

@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
Copy link
Member

rluvaton commented Sep 1, 2023

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

@atlowChemi
Copy link
Member

@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
Copy link
Contributor

cjihrig commented Sep 1, 2023

@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
Copy link
Member

MoLow commented Sep 3, 2023

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
Copy link
Member

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
Copy link
Member

MoLow commented Sep 3, 2023

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants