fix: stop printing the same error for each individual test caused by beforeAll when it fails

[vldmrkl]

Summary

Closes #9901

From my research, I found that the call of addErrorToEachTestUnderDescribe was causing this bug (line 160):
https://github.com/facebook/jest/blob/0a4e4a1657b42b21ebf3a969fbf3bbdd28ca930f/packages/jest-circus/src/eventHandler.ts#L154-L170

Removing this function call would stop printing error messages from beforeAll failure for each individual test, however, test suit would pass even though beforeAll failed. That's why I had to add an error to state.unhandledErrors, so it fails the test suit and prints an error message for beforeAll failure.

Since the code is the same as as-if statement for afterAll type, I joined type === 'beforeAll' and type === 'afterAll'.

These changes haven't broken any existing test in the project, and it looks like it is much less confusing comparing to the previous version, though I would like to get confirmation that this is the expected behaviour.

Test plan

Let's reuse the example from the issue description and see a new behaviour that this bug fix provides.

Example:

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 be reported as failed too
});

New behaviour:

 FAIL  ./beforeAllFails.test.js


  ● Test suite failed to run

    expect(received).toBe(expected) // Object.is equality

    Expected: "successful"
    Received: "login"

      1 | describe('test that a 3rd party API remains consistent', () => {
    > 2 |   beforeAll(() => expect('login').toBe('successful'));
        |                                   ^
      3 |   test('API function 1', () => expect(1).toBe(1));
      4 |   test('API function 2', () => expect(2).toBe(2)); // ...of these...
      5 |   test('API function 3', () => expect(3).toBe(3)); // ...will be reported as failed too

      at beforeAllFails.test.js:2:35

Test Suites: 1 failed, 1 total
Tests:       3 passed, 3 total
Snapshots:   0 total
Time:        3.559 s

[SimenB]

This has broken 2 tests. To run with circus locally, do JEST_CIRCUS=1 yarn jest.

Also, I think we should have better messages. Instead of unhandled we can have a new hookErrors field or something? That might be considered breaking for people consuming our events though, not sure.

This also needs to be covered by explicit tests 🙂

[thernstig]

@vldmrkl It says Tests: 3 passed, 3 total, should this not say Tests: 0 passed, 3 total? Maybe that is a separate issue though, related to #9911 ?

[github-actions]

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

[thernstig]

@vldmrkl any chance you could finalize this? :)

[vldmrkl]

@thernstig #9901 (comment)

[vldmrkl]

Closing this PR in favour of #13273

[github-actions]

This pull request 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.