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

detectOpenHandles imply runInBand #8283

Merged
merged 4 commits into from Apr 7, 2019

Conversation

Projects
None yet
4 participants
@jeysal
Copy link
Collaborator

commented Apr 7, 2019

Summary

detectOpenHandles makes no sense without runInBand, because it cannot detect leaks in workers.
I've also changed shouldRunInBand to just accept the config object, otherwise it would have lots of params.

Test plan

Added one test.each line. Also tested manually.

@jeysal jeysal requested review from thymikee and SimenB Apr 7, 2019

) {
/**
// detectOpenHandles makes no sense without runInBand, because it cannot detect leaks in workers

This comment has been minimized.

Copy link
@thymikee

thymikee Apr 7, 2019

Collaborator

because it cannot detect leaks in workers

Is this a limitation of async_hooks API?

Anyway, since detectOpenHandles flag is handled before initializing TestScheduler, how about moving this logic there and adding a proper error message so users are aware? We need to alarm users about that anyway.

I'm good passing whole config to shouldRunInBand 👍

This comment has been minimized.

Copy link
@jeysal

jeysal Apr 7, 2019

Author Collaborator

Is this a limitation of async_hooks API?

I mean, you could certainly make leak detection work in each worker, but I don't think it's worth it - a detectOpenHandles debug run doesn't need performance through parallelization.

adding a proper error message so users are aware? We need to alarm users about that anyway.

Not sure I understand correctly, do you want to error if detectOpenHandles without runInBand is used instead of just activating runInBand implicitly? I think forcing users to specify both would be quite annoying. Printing "did not exit within 1 second, please use --detectOpenHandles --runInBand" also doesn't look good :/

This comment has been minimized.

Copy link
@thymikee

thymikee Apr 7, 2019

Collaborator

On the second thought, we can hide it from the user. Would be nice to add a description note that adding this flag will make tests running serially

This comment has been minimized.

Copy link
@jeysal

jeysal Apr 7, 2019

Author Collaborator

Added to docs & yargs 👍

@SimenB

SimenB approved these changes Apr 7, 2019

@jeysal jeysal merged commit f435f01 into facebook:master Apr 7, 2019

10 of 11 checks passed

ci/circleci: test-jest-circus Your tests failed on CircleCI
Details
ci/circleci: lint-and-typecheck Your tests passed on CircleCI!
Details
ci/circleci: test-browser Your tests passed on CircleCI!
Details
ci/circleci: test-node-10 Your tests passed on CircleCI!
Details
ci/circleci: test-node-11 Your tests passed on CircleCI!
Details
ci/circleci: test-node-6 Your tests passed on CircleCI!
Details
ci/circleci: test-node-8 Your tests passed on CircleCI!
Details
ci/circleci: test-or-deploy-website Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
facebook.jest #20190407.7 succeeded
Details

@jeysal jeysal deleted the jeysal:detectopenhandles-imply-runinband branch Apr 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.