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.

Sign up for GitHub

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

Jump to bottom

afterAll hook hides errors. #3266

Closed
gdborton opened this issue Apr 7, 2017 · 11 comments
Closed

afterAll hook hides errors. #3266

gdborton opened this issue Apr 7, 2017 · 11 comments
Labels
🐛 Bug

Comments

@gdborton
Copy link
Contributor

gdborton commented Apr 7, 2017

Do you want to request a feature or report a bug?

Bug 🐛

What is the current behavior?

Errors thrown in afterAll are caught somewhere in Jest, and can cause a test suite to fail silently.

See the repl here - https://repl.it/HAYG/1

What is the expected behavior?

The afterAll hook should respect errors that are thrown, the same way that beforeAll does.

Please provide your exact Jest configuration and mention your Jest, node, yarn/npm version and operating system.

❯ node --version; npm --version; jest --version
v4.6.2
2.15.11
v19.0.0
@ljharb
Copy link
Contributor

ljharb commented Apr 7, 2017

In addition, this functionality is critical for global afterAll things like "fail the test if the test author failed to do some kind of critical cleanup task".

@aaronabramov
Copy link
Contributor

yes! we absolutely need this!
We inherited this behavior from jasmine, but now since we started the migration away from it we can actually fix it.

@ljharb
Copy link
Contributor

ljharb commented Apr 8, 2017

It could presumably be fixed upstream in jasmine as well?

@gdborton
Copy link
Contributor Author

gdborton commented Apr 8, 2017

Does jest not use jasmine internally? It looks like this works as intended in jasmine 2.5 at least.

@gdborton
Copy link
Contributor Author

So I dug into this a bit, and it looks like Jasmine is assigning most errors to Spec instances, but afterAll errors get assigned to Suite instances (same in Jasmine master).

It seems that Jest is only looking at exceptions that happen in Specs.

afterAll errors are handled here - https://github.com/facebook/jest/blob/master/packages/jest-jasmine2/src/jasmine/Suite.js#L156

Still looking for where Jest handles these.

@gdborton
Copy link
Contributor Author

gdborton commented Apr 10, 2017 •
edited
Loading

So, you can get the results of the suite here - https://github.com/facebook/jest/blob/master/packages/jest-jasmine2/src/reporter.js#L73

There is an unused argument passed to suiteDone that is the result of the suite, which looks like this...

{ id: 'suite1',
      description: 'Jasmine2Reporter',
      fullName: 'Jasmine2Reporter',
      failedExpectations: 
       [ { actual: '',
           error: 
            Error: asdf
                at Object.afterAll (/Users/gary_borton/airlab/repos/jest/packages/jest-jasmine2/src/__tests__/reporter-test.js:22:11)
                at Object.asyncFn (/Users/gary_borton/airlab/repos/jest/packages/jest-jasmine2/build/jasmine-async.js:40:30)
                at e (/Users/gary_borton/airlab/repos/jest/packages/jest-jasmine2/build/queueRunner.js:39:12)
                at mapper (/Users/gary_borton/airlab/repos/jest/packages/jest-jasmine2/build/queueRunner.js:32:21)
                at Promise.resolve.then.el (/Users/gary_borton/airlab/repos/jest/packages/jest-jasmine2/node_modules/p-map/index.js:42:16)
                at process._tickCallback (internal/process/next_tick.js:109:7),
           expected: '',
           matcherName: '',
           message: 'Error: asdf',
           passed: false,
           stack: 'Error: asdf\n    at Object.afterAll (/Users/gary_borton/airlab/repos/jest/packages/jest-jasmine2/src/__tests__/reporter-test.js:22:11)\n    at Object.asyncFn (/Users/gary_borton/airlab/repos/jest/packages/jest-jasmine2/build/jasmine-async.js:40:30)\n    at e (/Users/gary_borton/airlab/repos/jest/packages/jest-jasmine2/build/queueRunner.js:39:12)\n    at mapper (/Users/gary_borton/airlab/repos/jest/packages/jest-jasmine2/build/queueRunner.js:32:21)\n    at Promise.resolve.then.el (/Users/gary_borton/airlab/repos/jest/packages/jest-jasmine2/node_modules/p-map/index.js:42:16)\n    at process._tickCallback (internal/process/next_tick.js:109:7)' } ],
      status: 'failed' }

This doesn't match against the SpecResult type, nor does it have all the required fields for a AssertionResult. This is starting to look like it'll require some cascading changes (new types, formatting the new types into messages, teaching Jest about suite failures or mapping it to a spec failure).

I don't think I'd be able to PR this any time soon.

@vajahath
Copy link

any work around for this? 😕

@thymikee thymikee mentioned this issue Feb 16, 2018
Closed
@thymikee
Copy link
Collaborator

Different context on the same issue (marked as a duplicate): #3785

@rafaelramalho19
Copy link
Contributor

Working on it :)

@rafaelramalho19 rafaelramalho19 mentioned this issue Mar 27, 2018
Merged
@SimenB
Copy link
Member

SimenB commented Apr 9, 2018

#5884

@SimenB SimenB closed this as completed Apr 9, 2018
wchargin added a commit to sourcecred/sourcecred that referenced this issue Apr 27, 2018
@wchargin
Clean up temporary directories in tests
4657d11 
Summary:
The `loadRepository` test tries to clean up temporary directories, but
failed to do so because the directories were not empty. The cleanup hook
threw an error, but this error was silenced by Jest due to [a known
bug][1] that was fixed a few days ago. We can fix this by asking `tmp`
to clean up directories even if they are not empty, using the
`unsafeCleanup` option.

[1]: jestjs/jest#3266

Test Plan:
While running `watch -n 0.1 'ls /tmp | grep "tmp-.*" | wc -l'`, run
tests. Note that the number increases by five and then drops down again;
before this patch, it would increase by 5 and then stay there.

wchargin-branch: clean-up-tmpdirs
@wchargin wchargin mentioned this issue Apr 27, 2018
Merged
wchargin added a commit to sourcecred/sourcecred that referenced this issue Apr 27, 2018
@wchargin
Clean up temporary directories in tests (#151)
6f9941b 
Summary:
The `loadRepository` test tries to clean up temporary directories, but
failed to do so because the directories were not empty. The cleanup hook
threw an error, but this error was silenced by Jest due to [a known
bug][1] that was fixed a few days ago. We can fix this by asking `tmp`
to clean up directories even if they are not empty, using the
`unsafeCleanup` option.

[1]: jestjs/jest#3266

Test Plan:
While running `watch -n 0.1 'ls /tmp | grep "tmp-.*" | wc -l'`, run
tests. Note that the number increases by five and then drops down again;
before this patch, it would increase by 5 and then stay there.

wchargin-branch: clean-up-tmpdirs
@galenmaly galenmaly mentioned this issue Jul 13, 2018
Closed
@aalexgabi aalexgabi mentioned this issue Jan 6, 2020
Closed
@benbrimeyer benbrimeyer mentioned this issue Jan 16, 2020
Merged
@evandrocoan evandrocoan mentioned this issue Apr 25, 2020
Closed
@Izhaki Izhaki mentioned this issue Apr 29, 2020
Closed
@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 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
🐛 Bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@ljharb @aaronabramov @SimenB @gdborton @thymikee @vajahath @rafaelramalho19