Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Total assertion count expectations are inaccurate (with async expects) #8297

Open
ballercat opened this issue Apr 9, 2019 · 3 comments
Open

Total assertion count expectations are inaccurate (with async expects) #8297

ballercat opened this issue Apr 9, 2019 · 3 comments

Comments

@ballercat
Copy link

@ballercat ballercat commented Apr 9, 2019

馃悰 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 ballercat changed the title Total assertion Count expectations are inaccurate (with async expects) Total assertion count expectations are inaccurate (with async expects) Apr 9, 2019
@ballercat

This comment has been minimized.

Copy link
Author

@ballercat ballercat commented Apr 10, 2019

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

This comment has been minimized.

Copy link
Collaborator

@jeysal jeysal commented Apr 10, 2019

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 added a commit to ballercat/jest-plugin-must-assert that referenced this issue Apr 11, 2019
- Fail on missed assertions
- Workaround facebook/jest#8297 with zone.js in the plugin
@ballercat

This comment has been minimized.

Copy link
Author

@ballercat ballercat commented Apr 11, 2019

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can鈥檛 perform that action at this time.