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

Detect open handles with done callbacks #11382

Merged
merged 13 commits into from May 20, 2021
Merged
22 changes: 22 additions & 0 deletions e2e/__tests__/detectOpenHandles.ts
Expand Up @@ -123,3 +123,25 @@ 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 {stderr} = runJest('detect-open-handles', [
'in-done-function',
'--detectOpenHandles',
'--forceExit',
Copy link
Member

Choose a reason for hiding this comment

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

can you use waitUntil instead of forceExit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There doesn't seem to be any text that comes after the message about the open handle(s), so the best I could think to do with waitUntil() was to wait for the message that tests were done, then wait a few more ticks for the open handles to be printed. If I wait for the actual message about the open handles, the test would time out waiting in the error case when the handles aren't printed. --forceExit seemed cleaner and more reliable than that. Is there a better approach I'm missing?

Copy link
Member

Choose a reason for hiding this comment

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

I think it makes more sense to match the style of the other tests which will all time out if the message is not printed.

One thing we could do in general I guess is to add a timeout to waitUntil and first wait for tests to complete, then wait for the handle text with a timeout. But regardless, for consistency now I prefer not using forceExit

]);
const textAfterTest = getTextAfterTest(stderr);

expect(textAfterTest).toContain('TCPSERVERWRAP');
});

it('prints out info about open handlers from lifecycle functions with a `done` callback', async () => {
const {stderr} = runJest('detect-open-handles', [
'in-done-lifecycle',
'--detectOpenHandles',
'--forceExit',
]);
const textAfterTest = getTextAfterTest(stderr);

expect(textAfterTest).toContain('TCPSERVERWRAP');
});
15 changes: 15 additions & 0 deletions e2e/detect-open-handles/__tests__/in-done-function.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.
*/
const http = require('http');

test('something', done => {
const server = http.createServer((_, response) => response.end('ok'));
server.listen(0, () => {
expect(true).toBe(true);
done();
});
});
18 changes: 18 additions & 0 deletions e2e/detect-open-handles/__tests__/in-done-lifecycle.js
@@ -0,0 +1,18 @@
/**
* 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.
*/
const http = require('http');

beforeAll(done => {
const server = http.createServer((_, response) => response.end('ok'));
server.listen(0, () => {
done();
});
});

test('something', () => {
expect(true).toBe(true);
});
17 changes: 17 additions & 0 deletions packages/jest-core/src/__tests__/collectHandles.test.js
Expand Up @@ -6,6 +6,7 @@
*
*/

import http from 'http';
import {PerformanceObserver} from 'perf_hooks';
import collectHandles from '../collectHandles';

Expand Down Expand Up @@ -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();
});
});
});
});
27 changes: 23 additions & 4 deletions packages/jest-jasmine2/src/jasmineAsyncInstall.ts
Expand Up @@ -42,13 +42,22 @@ function promisifyLifeCycleFunction(
if (!fn) {
// @ts-expect-error: missing fn arg is handled by originalFn
return originalFn.call(env);
} else if (typeof fn !== 'function') {
Mr0grog marked this conversation as resolved.
Show resolved Hide resolved
// Pass non-functions to Jest, which throws a nice error.
return originalFn.call(env, fn, timeout);
}

const hasDoneCallback = typeof fn === 'function' && fn.length > 0;
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.
// @ts-expect-error: Support possible extra args at runtime
const asyncJestLifecycleWithCallback = function (this: any, ...args) {
Mr0grog marked this conversation as resolved.
Show resolved Hide resolved
// @ts-expect-error: Support possible extra args at runtime
return fn.apply(this, args);
};
return originalFn.call(env, asyncJestLifecycleWithCallback, timeout);
}

const extraError = new Error();
Expand Down Expand Up @@ -104,12 +113,22 @@ function promisifyIt(
const spec = originalFn.call(env, specName);
spec.pend('not implemented');
return spec;
} else if (typeof fn !== 'function') {
Mr0grog marked this conversation as resolved.
Show resolved Hide resolved
// 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.
// @ts-expect-error: Support possible extra args at runtime
const asyncJestTestWithCallback = function (this: any, ...args) {
// @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();
Expand Down