vitest icon indicating copy to clipboard operation
vitest copied to clipboard

Bail option to skip all subsequent tests

Open jgoux opened this issue 3 years ago • 5 comments

Clear and concise description of the problem

Depending on the kind of tests we run, it can be interesting to have a fail-fast feature and exit as soon as a test is failing.

I'm not the only one needing such feature apparently: https://github.com/vitest-dev/vitest/discussions/964

Jest has bail for this purpose.

Suggested solution

As Vitest tries to smooth the transition from Jest, I think the bail option is a sensible name.

But Jest implementation is incomplete/weird, it's only skipping the next test suites (describe blocks), but it continues to run the other tests within the same test suite.

I think the default should be to bail by test case and not by test suite.

# Skip all the subsequent tests after n test failures (default 1)
--bail <n>

I'm not sure what could be the API if we want to deal with both test cases and test suites, maybe adding a --bail-scope <test|suite> (default test)? 🤔

Alternative

No response

Additional context

Here are related issues about it:

  • https://github.com/facebook/jest/issues/8387
  • https://github.com/facebook/jest/issues/6527
  • https://github.com/facebook/jest/issues/2867

Validations

jgoux avatar Jun 09 '22 19:06 jgoux

Can you explain how you see it? For example, what should happen in those situations:

  1. test failed without describe
test('test 1', () => {/* code that fails */})
test('test 2', () => {/* other test */})
  1. test failed inside describe
describe('describe 1', () => {
  test('test 1', () => {/* code that fails */})
  test('test 2', () => {/* other test */})
})

describe('describe 2', () => {
  test('test 1', () => {/* other test */})
  test('test 2', () => {/* other test */})
})
  1. test failed inside nested describe
describe('describe 1', () => {
  describe('describe 2', () => {
    test('test 1', () => {/* code that fails */})
    test('test 2', () => {/* other test */})
  })

  test('test 1', () => {/* other test */})
  test('test 2', () => {/* other test */})
})
  1. test fails in one spec
// spec1.spec.ts
test('test 1', () => {/* code that fails */})
// spec2.spec.ts
test('test 2', () => {/* other test */})

sheremet-va avatar Jun 10 '22 10:06 sheremet-va

  1. test failed without describe
test('test 1', () => {/* code that fails */})
test('test 2', () => {/* other test */})

skip test 2 in both test and suite scopes.

  1. test failed inside describe
describe('describe 1', () => {
  test('test 1', () => {/* code that fails */})
  test('test 2', () => {/* other test */})
})

describe('describe 2', () => {
  test('test 1', () => {/* other test */})
  test('test 2', () => {/* other test */})
})

skip all the tests in test scope skip only "describe 1"."test 2" in suite scope

  1. test failed inside nested describe
describe('describe 1', () => {
  describe('describe 2', () => {
    test('test 1', () => {/* code that fails */})
    test('test 2', () => {/* other test */})
  })

  test('test 1', () => {/* other test */})
  test('test 2', () => {/* other test */})
})

skip all the tests in test scope skip only "describe 2"."test 2" in suite scope

  1. test fails in one spec
// spec1.spec.ts
test('test 1', () => {/* code that fails */})
// spec2.spec.ts
test('test 2', () => {/* other test */})

skip all the tests in test scope skip only other tests in spec1.spec.ts in suite scope

I think the rule should be:

  • using --bail-scope test, you see the whole tests as a flatten structure, if one test fail, no matter where it fails (in another file, in a describe, whatever) we skip everything else and display the report.
  • using --bail-scope suite, you skip the subsequent tests in the same "unit" as the failed test. Keep in mind that this "unit" can be logical, if you have 2 describe blocks named the same in two different test files, they are the same "unit". Also, it's more predictable to stop at the closest "unit", if you have nested describe, I don't think you want to skip the tests in the parent describe.

jgoux avatar Jun 10 '22 22:06 jgoux

Thank you for your clarification. I have some notes tho.

if you have 2 describe blocks named the same in two different test files, they are the same "unit"

This wouldn't be possible. One spec file knows nothing about the other, they are isolated (as they should be). And it doesn't really makes sense that test in one file can affect test in another.

I also don't like the name "test" for the scope. I personally get confused associating scope test with all tests, maybe we can choose a better name?

Also, should we have some escape hatch for specifying scope suite for some files, like describe.bail? Or that would be an overkill?

sheremet-va avatar Jun 11 '22 08:06 sheremet-va

This wouldn't be possible. One spec file knows nothing about the other, they are isolated (as they should be). And it doesn't really makes sense that test in one file can affect test in another.

I thought it was something possible with Jest, as you can nest describe blocks, it can make sense to have big groups to organize your tests (they don't really affect each other, it's more of an organization thing).

I also don't like the name "test" for the scope. I personally get confused associating scope test with all tests, maybe we can choose a better name?

Yes maybe test|suite is too vague, how about single|group?

Also, should we have some escape hatch for specifying scope suite for some files, like describe.bail? Or that would be an overkill?

It's a good idea but I would wait a bit for the initial implementation first and have more feedbacks about it.

jgoux avatar Jun 12 '22 11:06 jgoux

I thought it was something possible with Jest, as you can nest describe blocks, it can make sense to have big groups to organize your tests (they don't really affect each other, it's more of an organization thing).

As long as they are in the same file, it should work. Spec files don't communicate with each other.

Yes maybe test|suite is too vague, how about single|group?

single|group is more vague :D

I am ok with suite, but not sure about test, since in my head test is a smallest block of this hierarchy.

As far as I understand, test scope fails the whole tests run?

sheremet-va avatar Jun 12 '22 11:06 sheremet-va

May I ask if there is any news?

As already mentioned Jest provides a --bail flag And Playwright provides a --max-failures -x flag

It would save a lot of time in CI pipelines :)

jtuchel avatar Dec 13 '22 12:12 jtuchel

I'm integrating vitest into the lint-staged pipeline and replacing jest. Would be cool to bail early if there is any failed test.

vladcosorg avatar Dec 28 '22 13:12 vladcosorg

Any update on this? I am quite happy with vitest but it can add up a lot of cost for CI pipelines if you get an error in the first minute of execution but subsequent tests keep on running for another 15 minutes.

prafed avatar Feb 24 '23 11:02 prafed

Any update on this? I am quite happy with vitest but it can add up a lot of cost for CI pipelines if you get an error in the first minute of execution but subsequent tests keep on running for another 15 minutes.

PR welcome.

sheremet-va avatar Feb 24 '23 11:02 sheremet-va

PR welcome.

Add pr welcome label, just like it's added in other issues?

This post can be marked hidden as outdated once the label is added.

trivikr avatar Feb 27 '23 07:02 trivikr

Is there any specific place folks can look at to introduce --bail option?

For example, here is a function which marks failure for a task in a runner: https://github.com/vitest-dev/vitest/blob/895103ab0c75b86ed57f84424937eb9003a65c6e/packages/runner/src/run.ts#L198-L204

trivikr avatar Feb 27 '23 16:02 trivikr

I think vitest runner needs to check value of suite.result.state after it's run, and break the for loop with failure if bail option is set.

i.e. after line 327 in the following code https://github.com/vitest-dev/vitest/blob/895103ab0c75b86ed57f84424937eb9003a65c6e/packages/runner/src/run.ts#L315-L329

trivikr avatar Feb 27 '23 17:02 trivikr

I think vitest runner needs to check value of suite.result.state after it's run, and break the for loop with failure if bail option is set.

Feel free to take up this task, once maintainers confirm the solution posted above is the right way to go ahead or provide alternative suggestions.

I'll revisit this issue after a month or so, and can take up if it's not implemented by then.

trivikr avatar Feb 27 '23 17:02 trivikr

With the above proposed solution, the checks will have to be added at multiple levels.

For example, when the child suite is run concurrently https://github.com/vitest-dev/vitest/blob/895103ab0c75b86ed57f84424937eb9003a65c6e/packages/runner/src/run.ts#L253

Or the child suite is run in parallel https://github.com/vitest-dev/vitest/blob/895103ab0c75b86ed57f84424937eb9003a65c6e/packages/runner/src/run.ts#L265

Or the suite is run inside a child suite https://github.com/vitest-dev/vitest/blob/895103ab0c75b86ed57f84424937eb9003a65c6e/packages/runner/src/run.ts#L312

trivikr avatar Feb 27 '23 17:02 trivikr

I think the check needs to be done in runTest function:

  • Track number of failed tests in a global variable (say numberOfFailedTests), if bail option is set.
  • Increment numberOfFailedTests whenever a test fails. It can be done whenever failTask is called on test.result. A better alternative would be to create a new function which does this work - say failTest which does this work and calls failTask
    • https://github.com/vitest-dev/vitest/blob/895103ab0c75b86ed57f84424937eb9003a65c6e/packages/runner/src/run.ts#L152-L162
  • Skip running rest of the tests/suites if numberOfFailedTests is equal to or greater than one allowed by bail configuration.
    • return from runTest https://github.com/vitest-dev/vitest/blob/895103ab0c75b86ed57f84424937eb9003a65c6e/packages/runner/src/run.ts#L107
    • break from runFiles https://github.com/vitest-dev/vitest/blob/895103ab0c75b86ed57f84424937eb9003a65c6e/packages/runner/src/run.ts#L316-L317

If you're interested to take up this work, do wait for a comment from maintainers to check if there are better alternatives.

trivikr avatar Feb 27 '23 18:02 trivikr

If you're interested to take up this work, do wait for a comment from maintainers to check if there are better alternatives.

I don't know. Probably right. Be aware that files will only have one file if it's running with --threads:

https://github.com/vitest-dev/vitest/blob/895103ab0c75b86ed57f84424937eb9003a65c6e/packages/runner/src/run.ts#L316-L317

The hardest part here is to tell Vitest to stop running all other workers from within the worker without breaking Tinypool.

sheremet-va avatar Feb 27 '23 18:02 sheremet-va

The hardest part here is to tell Vitest to stop running all other workers from within the worker without breaking Tinypool.

Can we use SharedArrayBuffer to store numberOfFailedTests so that it can be read and updated across workers?

The blog post Using Shared Array Buffer in Node.js provides an example of how SharedArrayBuffer can be shared between workers.

I see that birpc is used for communication between workers? https://github.com/vitest-dev/vitest/blob/895103ab0c75b86ed57f84424937eb9003a65c6e/packages/vitest/src/node/pools/threads.ts#L23-L33

Can this store numberOfFailedTests value, or some support needs to be added in tinypool?

cc Tinypool author @Aslemammad for comments.

trivikr avatar Feb 27 '23 20:02 trivikr

Can we use SharedArrayBuffer to store numberOfFailedTests so that it can be read and updated across workers?

I think it's for use in worker_threads, but we also support child_process.

sheremet-va avatar Feb 27 '23 20:02 sheremet-va

but we also support child_process.

The numberOfFailedTests can be tracked by sending messages between parent and child using subprocess.send.

trivikr avatar Feb 27 '23 23:02 trivikr

It looks like the messages can be exchanged between worker threads or child processes using RuntimeRPC interfaced passed in createBirpc

  • Interface https://github.com/vitest-dev/vitest/blob/895103ab0c75b86ed57f84424937eb9003a65c6e/packages/vitest/src/types/rpc.ts#L9
  • child.ts https://github.com/vitest-dev/vitest/blob/895103ab0c75b86ed57f84424937eb9003a65c6e/packages/vitest/src/node/pools/child.ts#L19
  • https://github.com/vitest-dev/vitest/blob/895103ab0c75b86ed57f84424937eb9003a65c6e/packages/vitest/src/node/pools/threads.ts#L23

trivikr avatar Feb 27 '23 23:02 trivikr

After diving deep into the source code, I think the numberOfFailedTests can be tracked in Vitest core.

High level suggestion:

  • The variable can be similar to restartsCount (say failedTestsCount) https://github.com/vitest-dev/vitest/blob/bce5a9f8a8572805317ec336bb4289d18606679c/packages/vitest/src/node/core.ts#L47
  • A function can be added (say onTestFailed) which tracks the failed tests count, and calls .exit(true) or equivalent when the number goes over what's configured by bail.
  • The interface RuntimeRPC provides a function to call core function.
  • The RuntimeRPC function is called from failTest in runner.

If you're interested to take up this work, do wait for a comment from maintainers to check if there are better alternatives.

trivikr avatar Mar 01 '23 04:03 trivikr

I think this and #3128 could share the mechanic of how test running is stopped. Maybe there should be a way to tell all pools (and their test runners) to stop their execution gracefully. And when a test fails with --bail option, or in #3128's case user manually attempts to pause the execution in watch mode, this signal would be sent to pools.

  • When a pool receives this signal it should not start a new test file. It should let the current test file finish properly so that all afterEach cleanups etc are run.
  • When a runner receives this signal it should not start a new test case. It should let the current test case finish. Maybe every test/it should check whether an abort signal has been sent earlier before starting up.

AriPerkkio avatar Apr 04 '23 16:04 AriPerkkio