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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

failing beforeAll() causes even passing tests in the scope to fail #6695

Open
dandv opened this issue Jul 15, 2018 · 25 comments
Open

failing beforeAll() causes even passing tests in the scope to fail #6695

dandv opened this issue Jul 15, 2018 · 25 comments

Comments

@dandv
Copy link
Contributor

dandv commented Jul 15, 2018

馃悰 Bug Report

I'm looking for the best practice for aborting a describe block if the test setup fails. For example, when testing an API, if authenticating fails, it's pointless to run any other tests.

As suggested in the issue template, I searched StackOverflow first, where I found this incomplete answer, advising to place the initialization code in a beforeAll block. Hence, raising the issue here because

  1. I haven't seen anything documenting the practice of returning failure from beforeAll
  2. If a beforeAll block fails, tests in that describe block are still run, and they fail, even if otherwise they would pass!

To Reproduce

describe('test that a 3rd party API remains consistent', () => {
  beforeAll(() => expect('login').toBe('successful'));  // this will fail
  test('API function 1', () => expect(1).toBe(1));  // each...
  test('API function 2', () => expect(2).toBe(2));  // ...of these...
  test('API function 3', () => expect(3).toBe(3));  // ...will fail too
});

Expected behavior

Jest should report that the beforeAll() failed, and bail the describe scope without executing further tests in it. If it's somehow intended behavior that all tests should still be executed, and still marked as failed (which I find odd), this should be documented under beforeAll and in the Setup and Teardown guide. Other tests in the file, outside of the failing block, should still be executed.

Link to repl or repo (highly encouraged)

https://repl.it/@DanDascalescu/beforeAll-failure-should-bail-the-test

Run npx envinfo --preset jest

  System:
    OS: Linux 4.15 Ubuntu 16.04.4 LTS (Xenial Xerus)
    CPU: x64 Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz
  Binaries:
    Node: 10.5.0 - /usr/local/bin/node
    Yarn: 1.7.0 - /usr/bin/yarn
    npm: 6.2.0 - /usr/local/bin/npm
  npmPackages:
    jest: ^23.4.1 => 23.4.1 

See also

#6527

@SimenB
Copy link
Member

SimenB commented Jul 16, 2018

@aaronabramov have we changed the behaviour here for circus? Bailing after a failing lifecycle hook makes sense to me

@aaronabramov
Copy link
Contributor

yes! this was a bug in jasmine (or a feature). We changed this behavior in jest-circus.
@dandv jest-circus isn't shipped with jest by default yet, but you can test it by yarn add jest-circus and adding "testRunner": "jest-circus/runner" to your jest config

@alycda
Copy link

alycda commented Aug 17, 2018

I would like to add to this a request for named hooks. The test is marked as a failure and not run, but named hooks (especially when the hooks reside outside of the test file) would aid in debugging.

@SimenB
Copy link
Member

SimenB commented Aug 18, 2018

@alycda feel free to open up a separate issue for that 馃檪


Closing this as the behavior is fixed in jest-circus

@SimenB SimenB closed this as completed Aug 18, 2018
@medikoo
Copy link

medikoo commented Oct 18, 2018

Closing this as the behavior is fixed in jest-circus

It doesn't seem to be fixed in latest Jest (v23.6.0). So it's fixed for what users exactly? Facebook internal?

@SimenB
Copy link
Member

SimenB commented Oct 18, 2018

See the comment above: #6695 (comment)

Install circus (which will become the default in Jest at some point) and you should be good. Also see #7198

@medikoo
Copy link

medikoo commented Oct 18, 2018

Install circus (which will become the default in Jest at some point)

Thanks I'll follow. Still why then it's not set as default? Are there any other issues we should be aware of? Is jest-circus considered stable?

@SimenB
Copy link
Member

SimenB commented Oct 18, 2018

You can follow #6295 for state of making it the default.

levity added a commit to makerdao/dai.js that referenced this issue Mar 27, 2019
this solves the issue of seeing two failures for each test in test output when beforeAll fails

jestjs/jest#6695
levity added a commit to makerdao/dai.js that referenced this issue Mar 27, 2019
this solves the issue of seeing two failures for each test in test output when beforeAll fails

jestjs/jest#6695
@chrismwendt
Copy link

jest-circus seems to not fix this. All 3 tests still run and fail.

Here's the command I ran:

$ yarn run jest --testRunner=jest-circus/runner --bail

Am I missing a configuration value somewhere?

@dandv
Copy link
Contributor Author

dandv commented May 5, 2019

This is still as broken as ~1yr ago, even with circus... Please consider reopening.

image

@SimenB
Copy link
Member

SimenB commented May 6, 2019

To my eyes this just looks like an issue with the reporting. beforeAll is only run once and none of the other tests actually execute, but they all fail with the same reason, and they all print the same error.

We should probably avoid printing the individual tests if beforeAll fails. Right now the implementation just checks if we've had an error and if yes, print it and fail the test without actually executing it

@dandv
Copy link
Contributor Author

dandv commented May 9, 2019

I've seen a similar issue with reporting. It's hard to disentangle from proprietary code, but what happens is that a test.todo('description', callback); reports the MongoDB error thrown by the previously executed test (a bulk write failure) instead of the correct error that only a description should be specified for .todo tests. I tried throwing an error directly in that failing test but that doesn't reproduce the behavior.

Happy to see if the .todo issue reproduces once this is fixed.

Macil added a commit to InboxSDK/InboxSDK that referenced this issue May 9, 2019
@svyandun
Copy link

svyandun commented May 22, 2019

@SimenB the issue seems to be only partially fixed in circus since nested hooks are still being executed.

@thernstig
Copy link
Contributor

To my eyes this just looks like an issue with the reporting. beforeAll is only run once and none of the other tests actually execute, but they all fail with the same reason, and they all print the same error.

We should probably avoid printing the individual tests if beforeAll fails. Right now the implementation just checks if we've had an error and if yes, print it and fail the test without actually executing it

@SimenB Should we create a separate issue for this? Or was the intention to re-open this to continue the work in this one?

@SimenB
Copy link
Member

SimenB commented Apr 25, 2020

Sure, a new issue about the state today and what we want the end state to be makes sense to me

@pkuczynski
Copy link
Contributor

pkuczynski commented Dec 31, 2020

Is it a duplicate of #2713?

@dandv
Copy link
Contributor Author

dandv commented Feb 8, 2021

@pkuczynski I think so, yes.

@SimenB: my comment above describes the current state as of jest and jest-circus v26.6.3. I couldn't test v27 due to this ts-jest issue.

@Bec-k
Copy link

Bec-k commented Mar 17, 2021

Still not working as expected... Not via throwing an error or calling done.fail(err)

@github-actions
Copy link

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.

@JaneJeon
Copy link

Definitely not stale, still see this issue in latest jest.

@fider
Copy link

fider commented May 26, 2022

Undigging topic - imo. test suite should interrupt tests execution with appropriate message when one of hooks will fail or timeout (beforeEach/All, afterEach/All).

@github-actions
Copy link

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 May 26, 2023
@thernstig
Copy link
Contributor

Unstale

@dandv
Copy link
Contributor Author

dandv commented Sep 20, 2023

As a present for this issue's 5th birthday, I would like to ask a maintainer to reopen #2713, because throwing from beforeAll should really stop executing any further tests. @SimenB?

It's sad that 3rd party packages (now with tens of thousands of weekly downloads) had to be developed to work around this problem:

@vtgn
Copy link

vtgn commented Feb 19, 2024

It seems this framework is totally abandonned... RIP!

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

No branches or pull requests