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

[itaizelther]

Summary

Closes #9901

This is a continuation of @vldmrkl PR at #10004.

The call of addErrorToEachTestUnderDescribe is causing this bug:
https://github.com/facebook/jest/blob/264a116fd7e2bed4d3511fc85cf53226e7d9748a/packages/jest-circus/src/eventHandler.ts#L172-L174

This function adds an error to each of the tests under describe in which the failed beforeAll is. My currently proposed solution is changing this function operation to fail only one test and skip the rest, so that the error message should be printed once and all tests remain unrun.

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 behavior 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 behavior:

 FAIL  ./issue-9901.spec.ts
  test that a 3rd party API remains consistent
    × API function 1
    ○ skipped API function 2
    ○ skipped API function 3

  ● test that a 3rd party API remains consistent › API function 1

    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')); // this will fail
        |                                   ^
      3 |   test('API function 1', () => expect(1).toBe(1)); // each...
      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 Object.toBe (issue-9901.spec.ts:2:35)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 2 skipped, 3 total
Snapshots:   0 total
Time:        1.392 s
Ran all test suites.

This test is also added as an unit test on this PR.

[SimenB]

Exciting! I agree on skipping when a hook fails

1 failed, 2 skipped, 3 total

Seems wrong - all 3 were skipped

[itaizelther]

@SimenB In a matter of fact, it was my initial goal to set skip mode for all tests. But as it turns out, when all tests in a test suite are skipped, no error is printed, even though state has unhandled errors (like in afterAll hook).

Test Suites: 1 skipped, 0 of 1 total
Tests:       3 skipped, 3 total
Snapshots:   0 total
Time:        0.731 s, estimated 1 s
Ran all test suites.

So if we want to modify this behavior we'll have to edit source code that calls jest-circus. Per my understanding it's jest-reportes.

Should we pursue this solution? Or ignoring state errors when all tests fail is the desired behavior?

[SimenB]

when all tests in a test suite are skipped, no error is printed, even though state has unhandled errors

That sounds like a bug - although fixing it might be a semver major change. We have a concept of testExecError (e.g. syntax errors) - while it doesn't apply here there is a mechanism for non-test errors

[SimenB]

We can still land this in the meantime of course - it's strictly an improvement even if not "perfect"

[itaizelther]

That sounds like a bug - although fixing it might be a semver major change. We have a concept of testExecError (e.g. syntax errors) - while it doesn't apply here there is a mechanism for non-test errors

Well yes, that's how state errors are handled in jest-circus now:
https://github.com/facebook/jest/blob/f988721c02e8442b39d6dd92dba9bc2a8dd10ff0/packages/jest-circus/src/legacy-code-todo-rewrite/jestAdapterInit.ts#L197-L205

But the errors are ignored because all test suit is skipped. I can't seem to find what calls runAndTransformResultsToJestFormat to find the cause of this bug. Can you show me the previous step in this workflow?
btw, this exact same problem occurs when all tests are skipped and afterAll fails in production version. No error will be printed, because it is handled with a state error.

[itaizelther]

@SimenB Also, this is my first PR and it will be helpful to me if you told me if I should mention you each time I reply.

[SimenB]

But the errors are ignored because all test suit is skipped.

That sounds weird - could you post a small sample test that behaves this way?

@SimenB Also, this is my first PR and it will be helpful to me if you told me if I should mention you each time I reply.

The PR will bubble up on my notifications list regardless of the ping. Doesn't hurt, tho 🙂 I don't necessarily get a notification if you push code tho, so feel free to ping when you push 🙂

[itaizelther]

@SimenB

That sounds weird - could you post a small sample test that behaves this way?

Here you go:

describe('test that a 3rd party API remains consistent', () => {
  test.skip('API function 1', () => expect(1).toBe(1)); // skipped
  test.skip('API function 2', () => expect(2).toBe(2)); // skipped
  test.skip('API function 3', () => expect(3).toBe(3)); // skipped
  afterAll(() => expect('login').toBe('successfull')); // fails, should report failure
});

afterAll hook should be reported failed, but because all tests are meant to be skipped, the output ignores it.
When the tests are not skipped, it does report a failure.

So skipping all tests in beforeAll the same way won't print the error. It might be needed to be reported in another issue.

[itaizelther]

@SimenB
Have you had any chance to look into it?

[github-actions]

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

[itaizelther]

Hey, should I let this PR to close?

[SimenB]

Completely forgot about this PR, sorry!

[SimenB]

CI is failing - would you be able to take a look? Or is that waiting for #13273 (comment)?

[itaizelther]

CI is failing - would you be able to take a look? Or is that waiting for #13273 (comment)?

Yeah, it is.
Currently the PR marks the first test as failed, although we wish it to mark all as skipped, which prints output of skipped test case suite and no error.
I'll be glad if you could review my comments again and refer me to the relevant code or advise what should be the next steps here.

[vanshika0227]

can we merge the PR if it is done?

[itaizelther]

can we merge the PR if it is done?

Hey, as I have replied to @SimenB, this PR is yet done. There is an still an unfinished discussion about the PR implementation that I'd like one of the project maintainers could review so I can complete this feature.

Here's my last comment regarding the issue: #13273 (comment)

[github-actions]

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

[thernstig]

@itaizelther I just bumped the issue this links, since I assume this is still in the works?

[itaizelther]

Hey @thernstig,
This PR currently not in works.
I've wrote a piece printing the error only once as desired, but it marks other tests as "skipped", which is incorrect.
To fix that the change needs to be done fundamentally in the module calling jest-circus, but as @SimenB described it in the comments above, "fixing it might be a sever major change".

I suggest an experienced individual should point to the module that has to be refactored or take the PR to self .

[thernstig]

@SimenB is there a chance you have some guidance?