Failures in Test hooks like "AfterAll" don't get shown in the Junit Results

[mastrzyz]

Problem statement

There are situations where a testcase passes correctly but the afterAll block can fail, this currently leads to a Junit output that gives us a false sense that the test fully passes while Jest itself reports back the suite fails.

Repro

Here is a simple repro :

describe("Smoke Test", () => {
  afterAll(async () => {
    expect(1).toBe(2);
  });

  it("the truth", async () => {
    expect(1).toBe(1);
  });
});

Produces a JUNIT of

<?xml version="1.0" encoding="UTF-8"?>
<testsuites name="jest tests" tests="1" failures="0" errors="0" time="87.038">
  <testsuite name="Smoke Test" errors="0" failures="0" skipped="0" timestamp="2022-04-22T17:36:58" time="86.145" tests="1">
    <testcase classname="smoke.test.ts" name="Smoke Test the truth" time="0.003">
    </testcase>
  </testsuite>
</testsuites>

Jest itself outputs :

⌛ Finished test
Test Suites: 1 failed, 1 total
Tests:       1 passed, 1 total
Snapshots:   0 total
Time:        86.69 s

From the investigation that @palmerj3 made in this thread here

The community believes we should just create a new testcase for the failure.

Solution

Unfortunately Jest gives us the failure from the execution of the testHook in the testExecError field.

I've added an additional check if that field is not empty and then create a sort of fake testCase , to limit duplication I took the liberty of creating a testcase object generator function.

Open Questions

This is technically a bug right? so we shouldn't enable this via option but via a major version bump?

#211

[mastrzyz]

@palmerj3

[palmerj3]

I have one question so far.

On this line here: https://github.com/jest-community/jest-junit/pull/214/files#diff-98b33b9b4e6f46484fa6c752c587c61df6f8e15120262d850d62dfa5a55f2144R232

Say you have a syntax error in a test file or it simply cannot be parsed at all by Jest. I think it will still be caught and the description will read that it was a problem with the hook - but it may not be. Am I reading that right?

[mastrzyz]

With a repro of :

describe("Smoke Test", () => {
  it("should launch", async () => {
    Ilikestrains;
  });
});

Gives a testExecError of undefined

and actual test results :

[
  {
    "ancestorTitles": ["Smoke Test"],
    "duration": 4,
    "failureDetails": [{}],
    "failureMessages": ["ReferenceError: Ilikestrains is not defined"],
    "fullName": "Smoke Test should launch",
    "invocations": 3,
    "location": null,
    "numPassingAsserts": 0,
    "status": "failed",
    "title": "should launch"
  }
]

[mastrzyz]

And a repro of :

describe(" Smoke Test", () => {
  Ilikestrains;
  it("should launch", async () => {});
});

on the other hand will raise a Test hook execution failure

so kinda by design here right?

[palmerj3]

Yeah I just worry about stating it is a test hook execution failure if it really isn't the issue. Do you think we could make a more generic message such as, "Test execution failure: could be caused by afterAll hook or other hooks."?

[mastrzyz]

Done!

[mastrzyz]

@palmerj3 this look good now?

[palmerj3]

One small adjustment then I think I'm good to merge this in

[mastrzyz]

@palmerj3 does the small adjustment look good?

[palmerj3]

Please update this fixture to match what your logic now produces. Since the error message is now changed this and the test likely need to be updated right?