Establish consistent and reasonable failing hooks behaviour

[Izhaki]

Note: Lest I spend my time on a stale issue, I'm going to leave the details out. But after sweating more blood on the topic, I can lay a strong argument why beforeEach and afterEach should fail the test (not the block as described below). I'll be happy to share with the interested.

I don't think the jest architects have properly established the desired behaviour of what happens when hooks fail. And there are various issues related to this: #3266, #3785, #9368, #9901, to name a few.

I argue that the current behaviour is off and inconsistent, and the desired one is:

When a hook fails:

• Don't touch (modify) test results.
• Fail the suite

Then there's a separate discussion below on whether execution should stop if an after hook throws.

Current Behaviour

Jest: 25.5.0

The suite:

describe('block', () => {
  test('true', () => {
    expect(true).toBe(true);
  });
});

test('true', () => {
  expect(true).toBe(true);
});

And then the hook is added either outside or inside the describe block.

The hook (afterAll in this example) will look like this:

afterAll(()=>{
  throw new Error('Oh no!')
})

afterAll

Current:

hook	suite	tests
outside	1 failed	0 total
inside (bug #9368)	1 passed	2 passed

Desired:

hook	suite	tests
outside	1 failed	2 passed
inside	1 failed	2 passed

I'd like to start with this comment by @aaronabramov:

1. If i have a describe block with an afterAll hook and 10 tests, and all of my test pass, but afterAll hook throws, i'd expect all 10 tests to fail with the same error (taken from afterAll hook failure).

This is a bit like mixing apples and bananas. You should assert in tests, not hooks (which are conceptually for setup/teardown - not assertions).

If a test passed and an afterAll hook failed... well, the test passed.

Another way to look at it is that hooks should fail their parent (suite) not their siblings (tests).

Takeaway: Don't fuse tests and their surrounding hooks. Neither conceptually or in the way you handle failures.

The comment then goes on:

2. Given 1, i can not determine the state of the test (failed or passed) until i run all afterAll hooks for it and make sure they don't throw.
3. if i have a global afterAll hook i can only mark test as passed or failed after i run this hook, that means every single test's state resolution is blocked on this one global afterAll hook.

In the world of single process jasmine/mocha that would mean that you can't stream the results in real time, but rather wait for the whole test suite to be finished and then print the whole thing.

This smells. I don't think anyone would like results delayed until all the afterAll hooks are executed.

Also consider timing - how long does it take a test to run? Clearly you cannot include the time it takes the test to run + all the tests between it and the top-level afterAll.

What's more, the implementation involved is far more complex if we mark a test as failing if an afterAll hook as failed.

afterEach

Current:

hook	suite	tests
outside	1 failed	2 failed
inside	1 failed	1 failed, 1 passed

Desired:

hook	suite	tests
outside	1 failed	2 passed
inside	1 failed	2 passed

beforeAll

Current (bug #9901):

hook	suite	tests
outside	1 failed	2 failed
inside	1 failed	1 failed, 1 passed

(Note that the tests are marked as failed although none has been executed.)

Desired:

hook	suite	tests
outside	1 failed	2 skipped
inside	1 failed	1 skipped, 1 passed

beforeEach

Current:

hook	suite	tests
outside	1 failed	2 failed
inside	1 failed	1 failed, 1 passed

Desired:

hook	suite	tests
outside	1 failed	2 skipped
inside	1 failed	1 skipped, 1 passed

Desired Behaviour

RSpec

Going back to a succeeding comment by @aaronabramov, in rspec this is what happens when an afterAll hook fails:

Cucumber

It is worth remembering that in cucumber beforeAll and afterAll means before or after ALL the tests are/have been executed. That is, it is not scoped by the current module (suite) but rather by the whole run of all features.

• beforeAll:
  • Simply throws (no summary report)
• afterAll:
  • Runs all features (suites)
  • Stream results when tests are done
  • Throws (no summary report)
• before:
  • Catches and reports error
  • Scenario steps are marked as skipped
  • Scenario marked as failed
  • Execution continues
  • Summary: 9 scenarios (9 failed) 24 steps (24 skipped)
• after:
  • Runs the steps
  • Stream results
  • Catches and reports after error
  • Scenario marked as failed
  • Other features will execute
  • Summary: 9 scenarios (9 failed) 24 steps (24 passed)

Now this seems to me a very sensible strategy. But it leaves us with an open question.

Open questions

Should we stop execution or throw when a hook throws?

The case with before hooks is easy: If a before hook throws, neither tests (should be skipped) nor after hooks in the block should run. There is no issue with that.

The case with after hooks is slightly more complicated. Recall that afterAll hooks in jest are block-scoped (suite or describe). The issue can be illustrated with the following suite outline:

describe (1) {
  before (mount component)
  test
  after (unmount)
}

describe (2) {
  before (mount component)
  test
  after (unmount)
}

If describe(1) > unmount fails, we may have a dangling component rendered, and describe (2) > test will fail because it will pick the component mounted in describe (1) > before, rather then describe (2) > before.

A similar thing can happen if stubs are not restored in after hooks. And you can make a case for resources not being freed in some cases.

In other words, a failing after hook could yield an inconsistent test state.

So you could make an argument to re-throw exceptions in after hooks. Meaning the execution stops (or the whole process terminates with unhandled exception).

If after hooks exceptions are re-thrown, users can always explicitly wrap them with a try/catch block as in to say "It's OK for this after hook to fail". But that will prevent the suite from being marked as failed.

So really, what to do when after hooks fail is down to 3 choices:

1. Catch, fail suite, continue execution.
2. Catch, fail suite but stop execution (still provide summary).
3. Don't catch - execution will terminate with unhandled exception.

Option 2 seems most reasonable to me.

Lastly, the behaviour of failing after hooks can be determined by the fail-fast / bail option - if we are in fail-fast, any test or after hook failing will stop execution (option 2), otherwise continue (option 1). Related: #6527.

[github-actions]

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]

This issue was closed because it has been stalled for 30 days with no activity. Please open a new issue if the issue is still relevant, linking to this one.

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.