Total assertion count expectations are inaccurate (with async expects)

[ballercat]

🐛 Bug Report

expect.hasAssertions() and expect.assertions() incorrectly count assertions as part of a test when they should not be.

To Reproduce

The most basic example I could create to demonstrate

test('hasAssertions should fail expects in promises', () => {
  expect.hasAssertions();
  // Note that the expect below does not "count" as a failed assertion for this
  // test, but hasAssertions() also does not fail as it should!
  Promise.resolve().then(() => expect(true).toBe(false));
});

Expected behavior

The test above should fail, the async assertion should be ignored towards the assertion count. The promise chain, where the expect is executing is not part of the test. We can tell that because the test is not marked as failing even with a blatantly broken assertion. When the expect runs the test is "complete" and should not be counted as part of assertionCount in Jest.

Link to repl or repo (highly encouraged)

I forked the repo and wrote an integration test to demonstrate the problem. Branch

Permalink to the spec itself

https://repl.it/repls/DramaticCuteAssignments

Run npx envinfo --preset jest

❯ npx envinfo --preset jest

  System:
    OS: macOS High Sierra 10.13.6
    CPU: (4) x64 Intel(R) Core(TM) i5-5257U CPU @ 2.70GHz
  Binaries:
    Node: 8.11.1 - ~/.nvm/versions/node/v8.11.1/bin/node
    Yarn: 1.15.0 - /usr/local/bin/yarn
    npm: 5.6.0 - ~/.nvm/versions/node/v8.11.1/bin/npm

Notes

I do realize that the test is bad, it's not very well written. But tests like these do happen IRL and assertion count matchers is what you would want to use to catch them. Currently they do not work for this scenario.

This was pretty interesting and I dug into it a bunch, trying to see if there is an easy fix. But I was unable to locate an obvious mistake. It seems like the assertionCount logic (expect) is separate from the logic marking tests as failures (jest-jasmine2?) so there isn't a simple way to marry the two.

[ballercat]

This also has a negative effect on consecutive tests which use things like setTimeout to enqueue assertions in macrotasks. Leading to assertions being "leaked" across tests:

Example:

test.only('assertions after done() callback - 1', done => {
  setTimeout(() => {
    done();
    setTimeout(() => {
      expect(1 + 2).toBe(2);
    });
  });
});

test.only('assertions after done() callback - 2', done => {
  expect.hasAssertions();
  setTimeout(() => {
    done();
  });
});

This test setup leads to these results after tests complete:

 FAIL  src/__tests__/index.js
  ✓ assertions after done() callback - 1 (31ms)
  ✕ assertions after done() callback - 2 (6ms)

  ● assertions after done() callback - 2

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

    Expected: 2
    Received: 3

      28 |     done();
      29 |     setTimeout(() => {
    > 30 |       expect(1 + 2).toBe(2);
         |                     ^
      31 |     });
      32 |   });
      33 | });

      at Timeout.toBe [as _onTimeout] (src/__tests__/index.js:30:21)

Note that the second spec did not fail due to missing assertions, but instead due to a broken assertion enqueued from spec #1. I get what's going on and I get that it's due to the global nature of the expect library, but this is pretty surprising to see.

[jeysal]

Can reproduce this. IIRC at least for jest-circus at least one tick passes after the test until the result is evaluated because of internal awaits.
Not sure if this is easily fixable - you should at least see the unhandled rejection warning though to tell you there's something wrong with your test.
Note that the example from your comment is currently pretty much impossible to detect. I have experimented with using things like zone.js to implement some sort of strict async or debug mode for Jest that could detect this mistake, but nothing really concrete yet.

[ballercat]

Ooh, the zone.js library is very cool.

How I stumbled upon this is trying to write a "plugin" for jest which would catch these type of runtime issues. I have incorporated zone.js library into it as a PoC, and it seems to be working rather well. I'm going to throw this at a few thousand tests I have available and report back. Thanks!

[shrpne]

I have simillar issue.

My test:

test('should work', () => {
        expect.assertions(1);

        return getCoinInfo()
            .then((coinInfo) => {
                expect(coinInfo.id).toBeGreaterThan(0);
            })
            .catch((error) => {
                logError(error);
            });
    });

It should fail but it doesn't

[schw4rzlicht]

Running into this as well.

[ptim]

I've just stumbled across this issue due to my own misunderstanding of the behaviour of expect.hasAssertions and expect.assertions()

AFAICT, jest signals expectation failure by throwing, so when I run the initial example in my own code base, it fails, but the following test passes:

test('expect.assertions(2) should pass when caught', () => {
    expect.assertions(2)
    Promise.resolve()
      .then(() => {
        // jest signals expectation failure by thowing, and the intial example has no .catch
        expect(true).toBe(false)
      })
      .catch((e) => {
        expect(true).toBe(true)
      })
  })

(ack. it's highly likely things have changed since the issue was raised)

I'm unclear on why this works at all without returning or awaiting the promise 😄

You can chain as many Promises as you like and call expect at any time, as long as you return a Promise at the end.

https://jestjs.io/docs/tutorial-async (emphasis mine)

[SimenB]

Essentially same underlying issue here the solution I suggest in #9881 (comment) - expect is not bound to a single test, and maintains a global state. So async will confuse it. We need some way of binding an "instance" of expect to a single test, so we can reliably count assertions made in that specific test

[ballercat]

@SimenB Thanks for the response and I agree 100% RE: the problem of global expect not being bound to a single test.

It's probably a bit too late in the lifecycle of Jest to expand/change it's API in such a way but I for one would love to see an addition to the test(string, fn) API with something like test.strict that would directly provide the expect Fn as a variable instead of relying on the global. Similar ideas have worked for other test runners (ava comes to mind).

Ex:

test.strict('this test will fail assertions post test completion', (expect) => {
   setTimeout(() => expect(true).toBe(true), 10000);
});

[SimenB]

Yeah, I wanna add that type of API. Unfortunate we used the test name as just an alias for it instead of breaking teh API 😅

FWIW I'd me 100% open to a PR adding support for test.strict, test.bound, test.isolated or some such thing which binds/isolates expect. The name isn't the hard part, just need to figure out how to make expect not rely on global state (but still keep it to not break back compat), then how to support a callback (probably having a second arg be an options bag with {callback: boolean, timeout: number} or something)

[EllaMolnar-FOX]

FYI: A fix for this is to use done. For example:

  test('calls the API and throws an error', async (done) => {
    expect.assertions(2); // won't receive 3, because done() below prevents leaking
    const component = mount('Component');
    try {
      await component.login('email', 'password');
    } catch (error) {
      expect(error.name).toEqual('Unauthorized');
      expect(error.status).toEqual(401);
    }
    done();
  });
  test('calls the API and does not throw an error', async (done) => {
    expect.assertions(1); // not 3, because done() in previous test prevents the leaking
    const component = mount('Component');
    try {
      await component.login(user, password); // assuming these are set in the beforeEach()
    } catch (error) {
      // should not reach this -- user is magically known to be good
      console.log(error.msg)
    }
    expect(component.user).toBe(user);
    done();
  });

[ballercat]

@EllaMolnar-FOX

You are correct in that there are ways to write good tests which do not leak assertions. The examples provided in this issue are intentionally stripped down to reproducible example of the behavior. They are not intended as an example of a "good" test that still breaks.

As a side-note, the example above is perfectly valid with just async test functions without the done callback. Async functions return a promise by default (even if they resolve to an undefined value). Jest will wait for these to resolve, so the done is superfluous.

[mrbinky3000]

Unfortunately @EllaMolnar-FOX 's fix won't work for me because I'm using TypeScript for my tests. I can't use both async and done in the same test.

 error TS2345: Argument of type '(done: DoneCallback) => Promise<void>' is not assignable to parameter of type 'ProvidesCallback | undefined'.
      Type '(done: DoneCallback) => Promise<void>' is not assignable to type '(cb: DoneCallback) => void | undefined'.
        Type 'Promise<void>' is not assignable to type 'void'.