jestjs / jest

Delightful JavaScript Testing.
https://jestjs.io
MIT License
44.2k stars 6.46k forks source link

[Bug]: `jest --bail` doesn't call `globalTeardown` function if test fails. #14143

Open broyde opened 1 year ago

broyde commented 1 year ago

Version

29.5.0

Steps to reproduce

  1. Clone my repo at https://github.com/broyde/jest-teardown-bug
  2. yarn install
  3. yarn test
  4. You should see result like this:

    FAIL  ./jest.test.js
    Should call `globalTeardown` function before exit
    ✓ without bail (806 ms)
    ✕ with bail (659 ms)
    
    ● Should call `globalTeardown` function before exit › with bail

Optional steps (if you don't like jest.test.js - you could run fail.test.js directly):

  1. yarn jest fail.test.js

  2. You should see failed result followed by:

    **************************
    TEARDOWN
    **************************
  3. yarn jest fail.test.js --bail

  4. You should see the same fail, but without TEARDOWN log.

Expected behavior

yarn jest fail.test.js --bail outputs the TEARDOWN message, so yarn test passes:

$ jest --config={} jest.test.js
 PASS  ./jest.test.js
  Should call `globalTeardown` function before exit
    ✓ without bail (743 ms)
    ✓ with bail (696 ms)

Actual behavior

yarn jest fail.test.js --bail does not output the TEARDOWN message, so with bail case of yarn test fails:

$ jest --config={} jest.test.js
 FAIL  ./jest.test.js
  Should call `globalTeardown` function before exit
    ✓ without bail (750 ms)
    ✕ with bail (676 ms)

  ● Should call `globalTeardown` function before exit › with bail

    expect(received).toMatch(expected)

    Expected pattern: /TEARDOWN/
    Received string:  ""

      15 |   it('with bail', () => {
      16 |     expect(exec(['jest', 'fail.test.js', '--bail']))
    > 17 |       .toMatch(/TEARDOWN/);
         |        ^
      18 |   })
      19 | });
      20 |

      at Object.toMatch (jest.test.js:17:8)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 1 passed, 2 total
Snapshots:   0 total
Time:        1.612 s, estimated 2 s
Ran all test suites matching /jest.test.js/i

Additional context

This report is about the same bug as automatically closed https://github.com/jestjs/jest/issues/10607. Here I've created a hopefully better example with test which actually checks the jest behavior by running failing test without and with --bail option.

You could also run this example on Repl.

Reproduces on different environments and nodejs versions.

Environment

System:
    OS: Linux 5.15 Ubuntu 20.04.4 LTS (Focal Fossa)
    CPU: (6) x64 Intel(R) Core(TM) i5-8400 CPU @ 2.80GHz
  Binaries:
    Node: 16.17.1 - /usr/bin/node
    Yarn: 1.22.5 - /usr/bin/yarn
    npm: 8.15.0 - /usr/bin/npm
  npmPackages:
    jest: ^29.5.0 => 29.5.0
github-actions[bot] commented 1 year ago

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

noomorph commented 1 year ago

@SimenB wdyt about this hotfix (TestScheduler.ts and runJest.ts)?

image

Not the most beautiful one, but it should do the job... 🤔

noomorph commented 1 year ago

I stubmled upon this bug in Detox. Some of users tried to run tests with --bail and the results are horryifing 😭 the --bail logic disregards both the test environment cleanup and the global teardown.

I'll create a workaround in Detox to fight this. Perhaps, I'll be postponing the process.exit() inside onRunComplete via our Jest reporter override. Not a bright solution but I have no options if I want to maintain support for Jest 27+.

github-actions[bot] commented 1 year ago

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

broyde commented 1 year ago

This bug is also the reason why we can't use the jest-puppeteer's server option: the server remains running if some test fails with --bail :(

github-actions[bot] commented 1 year ago

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

noomorph commented 1 year ago

There is a pull request actually without a review, so no wonder this is still stale. ☹️

noomorph commented 1 year ago

@SimenB and the Jest team,

I genuinely appreciate and value the power and flexibility that Jest offers, however, the further I tinker with jest --bail implementation, the more disheartened I get.

I've found myself having to navigate intricate workarounds, particularly to ensure 1) that globalTeardown is called; 2) that Reporter#onRunComplete isn't invoked multiple times by different failing workers in --bail mode, especially when dealing with prolonged async operations.

1) https://github.com/wix/Detox/pull/4199/files#diff-4cf11a746383ff61964026f3a5fd6168b8b7695de2ff898977b79d411df6b3e1R34-R47

2) https://github.com/wix/Detox/pull/4115/files#diff-4cf11a746383ff61964026f3a5fd6168b8b7695de2ff898977b79d411df6b3e1R28-R30

While I've made efforts to address this with my proposed solution here, I'm beginning to feel it's merely a band-aid. I believe this issue needs a more comprehensive approach. I express this not as criticism but as feedback from someone deeply engaged with the tool. I hope you could share a few thoughts on this topic, especially if you're long aware of the design inconsistencies around --bail.

Thank you for your understanding and continued dedication to the Jest community.

github-actions[bot] commented 12 months ago

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

noomorph commented 12 months ago

Oh, yeah, of course it is stale.

github-actions[bot] commented 11 months ago

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

noomorph commented 11 months ago

🙄

github-actions[bot] commented 10 months ago

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

broyde commented 9 months ago

🙄

noomorph commented 9 months ago

@broyde did the idea with the reporter hack anyhow helped you, by the way?

broyde commented 9 months ago

@broyde did the idea with the reporter hack anyhow helped you, by the way?

@noomorph Jest doesn't want to stop the server (by calling globalTeardown), so the server has to stop by itself: Instead of using the server option in jest-puppeteer config, I directly run the test server. It then starts jest --bail as a sub-process and kills itself on sub-process' close event. I've been using this workaround for a few years and it doesn't really necessary to change it. So, I didn't try the reporter hack.

noomorph commented 9 months ago

Ah, that makes sense. Thanks for sharing!

github-actions[bot] commented 8 months ago

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

noomorph commented 7 months ago

This issue is still relevant.

github-actions[bot] commented 6 months ago

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

noomorph commented 6 months ago

Yes, it is still relevant

LizDodion commented 6 months ago

Still relevant and causeing issues where it does not clear up third parties

noomorph commented 6 months ago

@LizDodion just making sure you're aware it is possible to piggyback Reporter's onRunComplete(...) method for cleaning up. Of course, this is a suboptimal experience, but it may be helpful nevertheless.

github-actions[bot] commented 5 months ago

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

broyde commented 5 months ago

Still relevant (reproduced with jest v29.7.0).

github-actions[bot] commented 4 months ago

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

noomorph commented 4 months ago

Relevant as always.

github-actions[bot] commented 3 months ago

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

broyde commented 2 months ago

Still relevant.

noomorph commented 2 months ago

@broyde +1

Maybe the fact that Jest 30 is coming will justify some attention to this? 🤷‍♂️ It's safe enough to start experimenting with this in the major version context, anyways.

github-actions[bot] commented 1 month ago

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

broyde commented 1 month ago

Still relevant.

github-actions[bot] commented 5 days ago

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