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

feat: fail tests when multiple done() calls are made #10624

Merged
merged 22 commits into from
Dec 22, 2020

Conversation

flozender
Copy link
Contributor

@flozender flozender commented Oct 11, 2020

Summary

Aims to close #7052.

When done() is called more than once, the test is reported as a failure.

Changes made only to jest-circus as of now.

Test plan

e2e tests added to emulate the issue.

@flozender flozender changed the title fix: fail test when multiple done() calls are found feat: fail test when multiple done() calls are found Oct 11, 2020
@flozender flozender changed the title feat: fail test when multiple done() calls are found feat: fail tests when multiple done() calls are made Oct 11, 2020
CHANGELOG.md Outdated Show resolved Hide resolved
packages/jest-circus/src/utils.ts Outdated Show resolved Hide resolved
packages/jest-circus/src/utils.ts Outdated Show resolved Hide resolved
packages/jest-circus/src/utils.ts Outdated Show resolved Hide resolved
packages/jest-circus/src/utils.ts Outdated Show resolved Hide resolved
@flozender flozender requested a review from SimenB October 12, 2020 10:20
@flozender flozender requested a review from SimenB October 12, 2020 15:51
@flozender flozender requested a review from SimenB October 12, 2020 18:20
@flozender flozender requested a review from SimenB October 12, 2020 21:21
packages/jest-circus/src/utils.ts Outdated Show resolved Hide resolved
packages/jest-circus/src/eventHandler.ts Outdated Show resolved Hide resolved
packages/jest-circus/src/eventHandler.ts Outdated Show resolved Hide resolved
packages/jest-circus/src/run.ts Outdated Show resolved Hide resolved
packages/jest-circus/src/run.ts Outdated Show resolved Hide resolved
packages/jest-circus/src/utils.ts Outdated Show resolved Hide resolved
packages/jest-circus/src/utils.ts Outdated Show resolved Hide resolved
@SimenB
Copy link
Member

SimenB commented Oct 12, 2020

Changes made only to jest-circus as of now.

@flozender just noticed this in the OP - it's perfectly fine to just change jest-circus - jest-jasmine is more or less in maintenance mode at this point

@@ -212,6 +216,15 @@ export const callAsyncCircusFn = (

return reason ? reject(errorAsErrorObject) : resolve();
});

if (testOrHook.seenDone) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do it down here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to first check if done was passed with a reason for failure, if so, return the reason. Or else, we can check if done was called more than once and return that as an error. We have some tests that checked if reason was being sent out properly, and they were failing because we returned our Expected done to be called once, but it was called multiple times. error before the reason was checked.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which tests are those? If they call done multiple times that's wrong. I can take a look if there are cases it does make sense though - could you point me to the tests that failed?

Copy link
Contributor Author

@flozender flozender Oct 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's one such test that calls done more than once. EDIT: let me just check if my latest commit is causing the CI to fail and I'm just confused.

Copy link
Contributor Author

@flozender flozender Oct 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I was confused about something else, please ignore this. I'll revert the position-change commit. However, this doesn't work as expected: https://github.com/facebook/jest/blob/3641e3cc7309caf4c755add40bebcb77fc18b786/e2e/failures/__tests__/errorAfterTestComplete.test.js#L10-L13
Expects: Error: Caught error after test environment was torn down
Receives: Expected done to be called once, but it was called multiple times.
(Same error as in the CI)

UPD: So turns out I was trying to fix this issue by moving the seenDone check to after checking if the environment was torn down, hence the movement of the check. But clearly, that didn't work.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I'll merge this when we're ready to land breaking changes 🙂

@SimenB SimenB added this to the Jest 27 milestone Dec 5, 2020
@SimenB
Copy link
Member

SimenB commented Dec 5, 2020

@flozender CI is unhappy here - would you be able to take a look?

@flozender
Copy link
Contributor Author

Sure, I'll try to fix it.

@flozender
Copy link
Contributor Author

@SimenB I get an error here: https://github.com/facebook/jest/blob/a028bc13b8cb7d9f10933da67a41e7f52db277dd/packages/jest-jasmine2/src/jasmine/Env.ts#L56
56:31 error '@typescript-eslint/no-unused-vars' rule is disabled but never reported eslint-comments/no-unused-disable

Looks like it's a new change, what should I do here?

@SimenB
Copy link
Member

SimenB commented Dec 7, 2020

@flozender have you run yarn? It came from a recent dependency upgrade

@flozender
Copy link
Contributor Author

That didn't seem to work. I ran yarn in both the root directory and in jest-jasmine2.

@SimenB
Copy link
Member

SimenB commented Dec 7, 2020

Very odd. You can try to delete .eslintcache in root? CI is not failing with that lint warning

@flozender
Copy link
Contributor Author

Perfect, that worked!

@SimenB
Copy link
Member

SimenB commented Dec 8, 2020

Also please add a test like

test('something', done => {
  Promise.resolve().then(() => {
    done();
    done();
  });
});

packages/jest-circus/src/run.ts Outdated Show resolved Hide resolved
packages/jest-circus/src/utils.ts Outdated Show resolved Hide resolved
packages/jest-circus/src/utils.ts Outdated Show resolved Hide resolved
packages/jest-circus/src/utils.ts Outdated Show resolved Hide resolved
@SimenB SimenB merged commit 33b8df1 into jestjs:master Dec 22, 2020
@SimenB SimenB deleted the fail-done branch December 22, 2020 12:16
@SimenB
Copy link
Member

SimenB commented Dec 22, 2020

Thanks for a great contribution @flozender!

@github-actions
Copy link

This pull request 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 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"done" should fail when called multiple times
3 participants