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

Jest should fail fast and exit early (change request for --bail) #6527

Open
alycda opened this issue Jun 23, 2018 · 28 comments
Open

Jest should fail fast and exit early (change request for --bail) #6527

alycda opened this issue Jun 23, 2018 · 28 comments

Comments

@alycda
Copy link

alycda commented Jun 23, 2018

🚀 Feature Proposal

Jest should not run subsequent hooks or tests associated with that hook if an exception is thrown in a hook.

Motivation

When setup/teardown is broken up into small chunks (open browser, navigate to url, login, etc) and one of those hooks fails, all subsequent hooks and tests may fail and the output is clogged.

Example

When using Jest with puppeteer, I need to do the following before running any tests:

  1. open browser
  2. navigate to URL
  3. login
  4. wait for selector/page to be loaded

If any of those steps fail, the subsequent steps and tests are likely to fail as well.

Pitch

I'm using jest because of the detailed reporter, especially when a test fails. But failure reports can become needlessly long when it's only the first item that needs to be fixed to prevent a long subsequent list of failures/errors

I do not believe this is considered a change to the default reporter, as --bail does not behave the way I expect it to (it does stop after the first failing test, but if the test has 5 hooks, it still tries to run subsequent hooks and then the test fails).

@tangjeff0
Copy link

For those that want an immediate solution to this problem: #2867

@seeruk
Copy link

seeruk commented Jun 25, 2019

The suggested solution works, as long as you're not using jest-circus, you have to still be using Jasmine under the hood. Is there any word on this functionality making it's way into jest-circus as an option?

@lucasfcosta
Copy link
Contributor

Hello, everyone, I'm looking forward to helping with this if needed 😊

My impression is that this is actually a bug rather than a feature request.

It's also important to separate the two issues I see here:

  1. The one described by @seeruk, which is about the workaround described in Bug: --bail flag does not bail #2867 not working anymore. (This might not be a fault on Jest's side, but on how the workaround expected it to work)
  2. The fact that if a beforeAll/beforeEach hook fails with the --bail CLI option, the related tests still run.

I was able to reproduce (2) using [email protected].

describe("test bail", () => {
    // These will run regardless of the success of `beforeEach` or any other hooks
    beforeEach(() => { throw new Error("beforeEach hook fails!") });
    test("first", () => expect(true).toBe(true));
    test("second", () => expect(true).toBe(true));
    test("third", () => expect(true).toBe(true));
});

I think it's reasonable to expect that if beforeAll and beforeEach fail when using the --bail flag no subsequent tests in that same suite should run. IMO that's also what most people would expect to happen given the number of upvotes in this issue (but I'm happy to be proven wrong if that's the case).

For the sake of symmetry and coherence I think that the same should happen with afterAll and afterEach.

I'd be happy to send a PR for solving (2) if the maintainers agree that should be the desired behaviour 💖

@seeruk
Copy link

seeruk commented Jun 26, 2019

For the sake of symmetry and coherence I think that the same should happen with afterAll and afterEach.

It might only make sense for afterEach to fail the suite if --bail is used. afterAll is well... after all of the suite has run anyway.

I think my request is more of a feature request for circus. It'd be nice to be able to tell it to end a suite if a single test in it fails. My main use-case for this (which I'm using the workaround for with jest-jasmine currently) is E2E testing. There's not much point in continuing if an earlier test fails in a suite, because the rest will undoubtedly fail too. Then my custom reporter would spam Slack with all of the errors...

@SimenB
Copy link
Member

SimenB commented Jun 26, 2019

If beforeEach fails, the test should not run regardless of the bail flag. That's supposed to be the behaviour of Circus, but that's apparently broken (I'm on mobile, hard to track down the link).

We'll be removing Jasmine at some point (it'll stop being the default in the next major) so we're not really adding features to it, but happy to take a PR though if you wanna do the work :)


And I agree bail should work on the whole test run. Possibly behind an option if you want it to only short circuit per test file (current behaviour). Or switch the behaviour and make the current one be behind a new option

@lucasfcosta
Copy link
Contributor

@ALL @SimenB I'd love to try tackling this if possible 😊

If I understood correctly, as per your explanation here's a list of changes I intend to make if you agree they should be in:

  1. If beforeEach fails, the test should not run regardless of the bail flag
  2. Add a new flag for the current behaviour of bail which is to short-circuit per test file.
    This is what I'd recommend since to me it seems like bail should be general and there should be a second bailPerFile flag which encapsulates the current behaviour.

A third one would be to make #2867 work in jest-circus, but I'd like to focus on the two above first since I don't want to bite more than I can chew 😄

@seeruk
Copy link

seeruk commented Jun 27, 2019

@lucasfcosta, given the above suggestions, would --bail then stop a suite when the first test fails? That's the behaviour I'm pretty keen on. Thanks for looking into tackling these changes.

@lucasfcosta
Copy link
Contributor

lucasfcosta commented Jul 3, 2019

Just an update on this:

As I was investigating today, it seems like @SimenB was correct in regards to the tests not running if the beforeEach hook fails in jest-circus. However, in Jasmine, it still runs the tests even after beforeEach fails.

Running the following describe block without JEST_CIRCUS=1 will throw the errors inside each file:

describe('a block', () => {
  beforeEach(() => {
    throw new Error('The beforeEach hook failed.');
  });

  test('skipped first test', () => {
    throw new Error('First test should not have been run');
  });

  test('skipped second test', () => {
    throw new Error('Second test should not have been run');
  });
});

With jest-circus I get beforeEach hook fails! for each of the tests, but without it, I do get the respective error messages.

Given the explanation above, aborting tests in case the beforeEach hook fails is a fix which is only necessary for Jasmine.

I haven't yet got to the item number 2 in this comment but I'll keep updates contained in this issue (preferably editing this post).

PS: I'd rather report updates here since then I can link to the discussion in the PR and someone else can pick up the work if they need it, but if you think this is unnecessary noise in GH notifications just let me know and I'll avoid updates before the PR.

@krististepanek
Copy link

And I agree bail should work on the whole test run. Possibly behind an option if you want it to only short circuit per test file (current behaviour). Or switch the behaviour and make the current one be behind a new option

Is this change to the bail flag going to be implemented in jest-circus and is there somewhere that work is being tracked?

@aalexgabi
Copy link

I had a test case failing only when running after other test cases. I had to comment all following test cases to debug this because jest does not have an option to actually bail after 1 failed test case and not test suite.

@BorntraegerMarc
Copy link

Running npx jest --bail for the following file still prints out the console.log statements:

describe('a block', () => {
    beforeEach(() => {
        throw new Error('The beforeEach hook failed.');
    });

    test('skipped first test', () => {
        console.log('XXX skipped first test');
        // throw new Error('First test should not have been run');
        expect(true).toBe(true);
    });

    test('skipped second test', () => {
        console.log('XXX skipped second test');
        // throw new Error('Second test should not have been run');
        expect(true).toBe(true);
    });
});

Logs:

 FAIL  e2e/auth/test.e2e-spec.ts
  a block
    ✕ skipped first test (4ms)
    ✕ skipped second test (1ms)

  ● a block › skipped first test

    The beforeEach hook failed.

      1 | describe('a block', () => {
      2 |     beforeEach(() => {
    > 3 |         throw new Error('The beforeEach hook failed.');
        |               ^
      4 |     });
      5 | 
      6 |     test('skipped first test', () => {

      at Object.<anonymous> (e2e/auth/test.e2e-spec.ts:3:15)

  ● a block › skipped second test

    The beforeEach hook failed.

      1 | describe('a block', () => {
      2 |     beforeEach(() => {
    > 3 |         throw new Error('The beforeEach hook failed.');
        |               ^
      4 |     });
      5 | 
      6 |     test('skipped first test', () => {

      at Object.<anonymous> (e2e/auth/test.e2e-spec.ts:3:15)

  console.log e2e/auth/test.e2e-spec.ts:7
    XXX skipped first test

  console.log e2e/auth/test.e2e-spec.ts:13
    XXX skipped second test

This indicates to me that the tests are still run even though it says "skipped"

@MrLoh
Copy link

MrLoh commented May 1, 2020

It would be great to have a way to mark tests as dependent on previous step for e2e test as described here.

I think the simplest implementation might be a test.step function that will skip tests if a previous step in the describe has failed. I'd be happy to look into implementing this for jest-circus, but there needs to be agreement about the design first. This issue is not very specific about whether it treats beforeAll issues or just wants a way to mark tests as dependent.

@imankovskyi
Copy link

Consider using "bail" config option:

https://jestjs.io/docs/en/configuration#bail-number--boolean

This works for cases when you'd want a fast fail in beforeAll / beforeEach setup blocks.

@MrLoh
Copy link

MrLoh commented Jul 21, 2020

Not the same as already explained several times in these Threads

@wilsonpage
Copy link

@seeruk I've got a jest-circus solution I think works:

  1. Define CustomEnvironment.js
const JestEnvironmentNode = require("jest-environment-node");

class CustomEnvironment extends JestEnvironmentNode {
  /**
   * @param {import('jest-circus').Event} event
   * @param {import('jest-circus').State} state
   */
  async handleTestEvent(event, state) {
    if (event.name === 'test_fn_failure' || event.name === 'hook_failure') {
      // re-throw the error to exit immediately
      throw new Error(event.error);
    }
  }
}

module.exports = CustomEnvironment;
  1. Use Custom Environment in jest.config.js
module.exports = {
  testRunner: 'jest-circus/runner',
  testEnvironment: './CustomEnvironment.js',};

@davidglezz
Copy link

davidglezz commented Jan 14, 2021

Skip all after first failure

Before:
image
After:
image

I run test with --runInBand

  1. Define a custom Environment: jest-environment-fail-fast.js
const ParentEnvironment = require('jest-environment-node') 
// or require('jest-environment-jsdom')
// or require('jest-playwright-preset/lib/PlaywrightEnvironment').default

class JestEnvironmentFailFast extends ParentEnvironment {
    failedTest = false;
    
    async handleTestEvent(event, state) {
        if (event.name === 'hook_failure' || event.name === 'test_fn_failure') {
            this.failedTest = true;
        } else if (this.failedTest && event.name === 'test_start') {
            event.test.mode = 'skip';
        }

        if (super.handleTestEvent) {
            await super.handleTestEvent(event, state)
        }
    }
}

module.exports = JestEnvironmentFailFast
  1. In jest.config.js, testRunner must be 'jest-circus/runner' and:
    testEnvironment: './jest-environment-fail-fast.js',

More events names in https://github.com/facebook/jest/blob/master/packages/jest-circus/src/run.ts, dispatch functions.

@arlowhite
Copy link

This jasmine-fail-fast hack still works: #2867 (comment)

@brady-ds
Copy link

brady-ds commented Jul 24, 2021

@davidglezz Surely you mean

await super.handleTestEvent(event, state)

in your environment code, right?

(Edit: Thanks for applying the fix!)

@rcollette
Copy link

Totally agree with #6527 (comment).

Tests should stop running if before... has an exception/error. Your test setup conditions failed so how can you possibly run the expectations?

@vjjft
Copy link

vjjft commented Mar 10, 2023

For the solution posted by @davidglezz, it seems that [(for jest-environment-node, at minimum)] it is necessary1 to add .TestEnvironment for it to work properly.2

(edited as indicated to add clarification of context for the guidance)

Footnotes

  1. [I was using the solution in the context of jest-environment-node, and] without that addition I encountered a runtime error, while with the addition it worked as [I] expected/desired.

  2. This is supported by https://github.com/facebook/jest/blob/26bd03a77d6b683632adaf0a45ec912c0cd707a0/e2e/test-environment-circus/CircusHandleTestEventEnvironment.js#L10

@zirkelc
Copy link

zirkelc commented Mar 10, 2023

@vjjft in @davidglezz solution, it imports jest-environment-node while in your provided link it imports jest-environment-jsdom. I didn't check the implementation of both packages, but I guess that's the reason for the difference you encountered.

@vjjft
Copy link

vjjft commented Mar 10, 2023

@zirkelc I was/am only working with jest-environment-node, so I believe it is needed in both cases. I have edited my previous comment to provide that context.

@vjjft
Copy link

vjjft commented Mar 10, 2023

In case someone using Jest with Typescript would like a TS native solution adapted from the #6527 (comment) by @davidglezz, this is working for me:

import type { Circus, } from "@jest/types"
import { TestEnvironment, } from "jest-environment-node"
// import TestEnvironment from "jest-environment-jsdom"

class FailFastEnvironment extends TestEnvironment
{
  failedTest = false

  async handleTestEvent(
    event: Circus.Event,
    state: Circus.State,
  )
  {
    if ( event.name === "hook_failure" || event.name === "test_fn_failure" )
    {
      this.failedTest = true
    } else if ( this.failedTest && event.name === "test_start" )
    {
      event.test.mode = "skip"
    }

    // @ts-ignore
    if ( super.handleTestEvent ) await super.handleTestEvent( event, state, )
  }
}

export default FailFastEnvironment

@jeanhdev
Copy link

jeanhdev commented Apr 5, 2023

@vjjft it works perfectly ! clean and type-safe. thank you.

Copy link

github-actions bot commented Apr 4, 2024

This issue is stale because it has been open for 1 year with no activity. Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the Stale label Apr 4, 2024
@gladykov
Copy link

gladykov commented Apr 4, 2024

Unstale

@github-actions github-actions bot removed the Stale label Apr 4, 2024
@jdfm
Copy link

jdfm commented Apr 18, 2024

Should there just be a --full-bail option along with the --bail option? I think my confusion about how --bail works revolves around what test suite means, maybe this is similar to other peoples experience. In my head, it meant all of the tests associated to some project. But, it seems that it actually means all of the tests in a single file. So, using --bail may fail a specific file's test collection quickly, but, it doesn't do anything when it comes to the tests in other files in the same project.

@rogerprz
Copy link

Any updates on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.