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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"done" should fail when called multiple times #7052

Closed
martypdx opened this issue Sep 26, 2018 · 9 comments · Fixed by #10624
Closed

"done" should fail when called multiple times #7052

martypdx opened this issue Sep 26, 2018 · 9 comments · Fixed by #10624

Comments

@martypdx
Copy link

martypdx commented Sep 26, 2018

I did some online searching and didn't find any discussion, so let me know if this is as expected. Happy to do a PR if this should be correct behavior

🐛 Bug Report

Calling done() multiple times in one test should be a failure, but it succeeds

To Reproduce

it('should fail when done called second time', done => {
  done();
  done();
});

Expected behavior

Test fails with "done" called multiple times or some such message

Link to repl or repo (highly encouraged)

Above code snippet showcases issue. Looks like following test could be added to e2e failures:

test('done();done()', done => {
  done();
  done();
});

Run npx envinfo --preset jest

Paste the results here:

  System:
    OS: macOS High Sierra 10.13.6
    CPU: x64 Intel(R) Core(TM) i5-5287U CPU @ 2.90GHz
  Binaries:
    Node: 10.3.0 - /usr/local/bin/node
    Yarn: 1.5.1 - /usr/local/bin/yarn
    npm: 6.1.0 - /usr/local/bin/npm
@martypdx martypdx changed the title done should fail when called multiple times "done" should fail when called multiple times Sep 26, 2018
@SimenB
Copy link
Member

SimenB commented Sep 27, 2018

I agree this should fail. Wanna send a PR for it? 😀

@natealcedo
Copy link

Hey, I'd like to take this up. Don't have enough context as it is though. From what I understand, this will have to be updated in both circus and jasmine(Env.js?). Could you give me a place to start?

@natealcedo
Copy link

natealcedo commented Sep 29, 2018

The code for circus looks like something I can work with. The one for jasmine I don't quite grok. I don't see where the callback is provided. Basically what I plan on doing is to wrap done in a higher order function which increments a variable for done being called more than once. If it's called more than once then we mark the test as a fail.

@jmcsilva98
Copy link

Hello, me and another student are trying to fix this issue for a course in college. However, it's our first time working with jest so we are not so sure which files to run to reproduce this issue... Can you please replicate it in repl.it?

@rickhanlonii
Copy link
Member

rickhanlonii commented Nov 28, 2018

Hey @jmcsilva98, really happy to hear that!

Here's a repl.it, if you run this then the test passes but the OP requests that we fail it. The message could be something like "Expected done to be called once but it was called multiple times"

Here is a good place to start looking to add the feature. Note that this is the jest-circus runner so you should follow these instructions to run

Finally, here are some instructions on how to setup jest locally as well. These docs aren't our best, so if you have any questions, feel free to reach out to me on twitter or the reactiflux discord and I'd be happy to help!

@flozender
Copy link
Contributor

May I give this a shot?

@SimenB
Copy link
Member

SimenB commented Oct 11, 2020

That would be great, thanks!

@github-actions
Copy link

This issue 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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants