From 57e32e99a7994dfdcca8f777a2961294919551b9 Mon Sep 17 00:00:00 2001 From: Rob Brackett Date: Thu, 20 May 2021 06:10:03 -0700 Subject: [PATCH] Detect open handles with done callbacks (#11382) --- CHANGELOG.md | 2 ++ .../__snapshots__/detectOpenHandles.ts.snap | 32 +++++++++++++++++ e2e/__tests__/detectOpenHandles.ts | 32 +++++++++++++++++ .../__tests__/in-done-function.js | 12 +++++++ .../__tests__/in-done-lifecycle.js | 15 ++++++++ .../src/__tests__/collectHandles.test.js | 17 +++++++++ .../jest-jasmine2/src/jasmineAsyncInstall.ts | 35 ++++++++++++++++--- 7 files changed, 141 insertions(+), 4 deletions(-) create mode 100644 e2e/detect-open-handles/__tests__/in-done-function.js create mode 100644 e2e/detect-open-handles/__tests__/in-done-lifecycle.js diff --git a/CHANGELOG.md b/CHANGELOG.md index a5be224c3b4e..bb8daded58bc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -68,6 +68,7 @@ - `[jest-console]` `console.dir` now respects the second argument correctly ([#10638](https://github.com/facebook/jest/pull/10638)) - `[jest-core]` Don't report PerformanceObserver as open handle ([#11123](https://github.com/facebook/jest/pull/11123)) - `[jest-core]` Use `WeakRef` to hold timers when detecting open handles ([#11277](https://github.com/facebook/jest/pull/11277)) +- `[jest-core]` Correctly detect open handles that were created in test functions using `done` callbacks ([#11382](https://github.com/facebook/jest/pull/11382)) - `[jest-diff]` [**BREAKING**] Use only named exports ([#11371](https://github.com/facebook/jest/pull/11371)) - `[jest-each]` [**BREAKING**] Ignore excess words in headings ([#8766](https://github.com/facebook/jest/pull/8766)) - `[jest-each]` Support array index with template strings ([#10763](https://github.com/facebook/jest/pull/10763)) @@ -82,6 +83,7 @@ - `[jest-haste-map]` Vendor `NodeWatcher` from `sane` ([#10919](https://github.com/facebook/jest/pull/10919)) - `[jest-jasmine2]` Fixed the issue of `beforeAll` & `afterAll` hooks getting executed even if it is inside a skipped `describe` block when it has child `tests` marked as either `only` or `todo` [#10451](https://github.com/facebook/jest/issues/10451) - `[jest-jasmine2]` Fixed the issues of child `tests` marked with `only` or `todo` getting executed even if it is inside a skipped parent `describe` block [#10451](https://github.com/facebook/jest/issues/10451) +- `[jest-jasmine2]` Wrap all test functions so they open handles that were created in test functions using `done` callbacks can be detected ([#11382](https://github.com/facebook/jest/pull/11382)) - `[jest-reporter]` Handle empty files when reporting code coverage with V8 ([#10819](https://github.com/facebook/jest/pull/10819)) - `[jest-resolve]` Replace read-pkg-up with escalade package ([#10781](https://github.com/facebook/jest/pull/10781)) - `[jest-resolve]` Disable `jest-pnp-resolver` for Yarn 2 ([#10847](https://github.com/facebook/jest/pull/10847)) diff --git a/e2e/__tests__/__snapshots__/detectOpenHandles.ts.snap b/e2e/__tests__/__snapshots__/detectOpenHandles.ts.snap index 54666f5a111c..f679a6e076f5 100644 --- a/e2e/__tests__/__snapshots__/detectOpenHandles.ts.snap +++ b/e2e/__tests__/__snapshots__/detectOpenHandles.ts.snap @@ -38,3 +38,35 @@ Jest has detected the following 1 open handle potentially keeping Jest from exit at Object.setTimeout (__tests__/inside.js:9:3) `; + +exports[`prints out info about open handlers from lifecycle functions with a \`done\` callback 1`] = ` +Jest has detected the following 1 open handle potentially keeping Jest from exiting: + + ● Timeout + + 7 | + 8 | beforeAll(done => { + > 9 | setTimeout(() => {}, 10000); + | ^ + 10 | done(); + 11 | }); + 12 | + + at setTimeout (__tests__/in-done-lifecycle.js:9:3) +`; + +exports[`prints out info about open handlers from tests with a \`done\` callback 1`] = ` +Jest has detected the following 1 open handle potentially keeping Jest from exiting: + + ● Timeout + + 7 | + 8 | test('something', done => { + > 9 | setTimeout(() => {}, 10000); + | ^ + 10 | expect(true).toBe(true); + 11 | done(); + 12 | }); + + at Object.setTimeout (__tests__/in-done-function.js:9:3) +`; diff --git a/e2e/__tests__/detectOpenHandles.ts b/e2e/__tests__/detectOpenHandles.ts index 977434965a3f..f070f4b9e25b 100644 --- a/e2e/__tests__/detectOpenHandles.ts +++ b/e2e/__tests__/detectOpenHandles.ts @@ -123,3 +123,35 @@ it('prints out info about open handlers from inside tests', async () => { expect(wrap(textAfterTest)).toMatchSnapshot(); }); + +it('prints out info about open handlers from tests with a `done` callback', async () => { + const run = runContinuous('detect-open-handles', [ + 'in-done-function', + '--detectOpenHandles', + ]); + await run.waitUntil(({stderr}) => stderr.includes('Jest has detected')); + const {stderr} = await run.end(); + const textAfterTest = getTextAfterTest(stderr); + + expect(wrap(textAfterTest)).toMatchSnapshot(); +}); + +it('prints out info about open handlers from lifecycle functions with a `done` callback', async () => { + const run = runContinuous('detect-open-handles', [ + 'in-done-lifecycle', + '--detectOpenHandles', + ]); + await run.waitUntil(({stderr}) => stderr.includes('Jest has detected')); + const {stderr} = await run.end(); + let textAfterTest = getTextAfterTest(stderr); + + // Circus and Jasmine have different contexts, leading to slightly different + // names for call stack functions. The difference shouldn't be problematic + // for users, so this normalizes them so the test works in both environments. + textAfterTest = textAfterTest.replace( + 'at Object.setTimeout', + 'at setTimeout', + ); + + expect(wrap(textAfterTest)).toMatchSnapshot(); +}); diff --git a/e2e/detect-open-handles/__tests__/in-done-function.js b/e2e/detect-open-handles/__tests__/in-done-function.js new file mode 100644 index 000000000000..655a5c93ad80 --- /dev/null +++ b/e2e/detect-open-handles/__tests__/in-done-function.js @@ -0,0 +1,12 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +test('something', done => { + setTimeout(() => {}, 10000); + expect(true).toBe(true); + done(); +}); diff --git a/e2e/detect-open-handles/__tests__/in-done-lifecycle.js b/e2e/detect-open-handles/__tests__/in-done-lifecycle.js new file mode 100644 index 000000000000..1c4f2456491c --- /dev/null +++ b/e2e/detect-open-handles/__tests__/in-done-lifecycle.js @@ -0,0 +1,15 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +beforeAll(done => { + setTimeout(() => {}, 10000); + done(); +}); + +test('something', () => { + expect(true).toBe(true); +}); diff --git a/packages/jest-core/src/__tests__/collectHandles.test.js b/packages/jest-core/src/__tests__/collectHandles.test.js index 24644ef39215..7192709d9350 100644 --- a/packages/jest-core/src/__tests__/collectHandles.test.js +++ b/packages/jest-core/src/__tests__/collectHandles.test.js @@ -6,6 +6,7 @@ * */ +import http from 'http'; import {PerformanceObserver} from 'perf_hooks'; import collectHandles from '../collectHandles'; @@ -33,4 +34,20 @@ describe('collectHandles', () => { expect(openHandles).toHaveLength(0); obs.disconnect(); }); + + it('should collect handles opened in test functions with `done` callbacks', done => { + const handleCollector = collectHandles(); + const server = http.createServer((_, response) => response.end('ok')); + server.listen(0, () => { + // Collect results while server is still open. + const openHandles = handleCollector(); + + server.close(() => { + expect(openHandles).toContainEqual( + expect.objectContaining({message: 'TCPSERVERWRAP'}), + ); + done(); + }); + }); + }); }); diff --git a/packages/jest-jasmine2/src/jasmineAsyncInstall.ts b/packages/jest-jasmine2/src/jasmineAsyncInstall.ts index 30748c0461a6..be33f5b41bbc 100644 --- a/packages/jest-jasmine2/src/jasmineAsyncInstall.ts +++ b/packages/jest-jasmine2/src/jasmineAsyncInstall.ts @@ -44,11 +44,24 @@ function promisifyLifeCycleFunction( return originalFn.call(env); } - const hasDoneCallback = typeof fn === 'function' && fn.length > 0; + if (typeof fn !== 'function') { + // Pass non-functions to Jest, which throws a nice error. + return originalFn.call(env, fn, timeout); + } + + const hasDoneCallback = fn.length > 0; if (hasDoneCallback) { - // Jasmine will handle it - return originalFn.call(env, fn, timeout); + // Give the function a name so it can be detected in call stacks, but + // otherwise Jasmine will handle it. + const asyncJestLifecycleWithCallback = function ( + this: Global.TestContext, + ...args: Array + ) { + // @ts-expect-error: Support possible extra args at runtime + return fn.apply(this, args); + }; + return originalFn.call(env, asyncJestLifecycleWithCallback, timeout); } const extraError = new Error(); @@ -106,10 +119,24 @@ function promisifyIt( return spec; } + if (typeof fn !== 'function') { + // Pass non-functions to Jest, which throws a nice error. + return originalFn.call(env, specName, fn, timeout); + } + const hasDoneCallback = fn.length > 0; if (hasDoneCallback) { - return originalFn.call(env, specName, fn, timeout); + // Give the function a name so it can be detected in call stacks, but + // otherwise Jasmine will handle it. + const asyncJestTestWithCallback = function ( + this: Global.TestContext, + ...args: Array + ) { + // @ts-expect-error: Support possible extra args at runtime + return fn.apply(this, args); + }; + return originalFn.call(env, specName, asyncJestTestWithCallback, timeout); } const extraError = new Error();