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

fix(jest-worker): failed assertion with circular value hangs the test run #14675

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
- [**BREAKING**] Changes `testPathPattern` configuration option to `testPathPatterns`, which now takes a list of patterns instead of the regex.
- [**BREAKING**] `--testPathPattern` is now `--testPathPatterns`
- `[jest-reporters, jest-runner]` Unhandled errors without stack get correctly logged to console ([#14619](https://github.com/facebook/jest/pull/14619))
- [jest-worker] Properly handle a circular reference error when worker tries to send an assertion fails where either the expected or actual value is circular ([#14675](https://github.com/jestjs/jest/pull/14675))

### Performance

Expand Down
4 changes: 2 additions & 2 deletions e2e/__tests__/circularInequality.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ afterEach(() => {
cleanup(tempDir);
});

test.skip('handles circular inequality properly', async () => {
test('handles circular inequality properly', async () => {
const testFileContent = `
it('test', () => {
const foo = {};
Expand All @@ -44,7 +44,7 @@ test.skip('handles circular inequality properly', async () => {
tempDir,
['--no-watchman', '--watch-all'],
// timeout in case the `waitUntil` below doesn't fire
{stripAnsi: true, timeout: 5000},
{stripAnsi: true, timeout: 10_000},
);

await waitUntil(({stderr}) => stderr.includes('Ran all test suites.'));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,12 @@ import ThreadsWorker from '../NodeThreadsWorker';
jest.setTimeout(10_000);

const root = join('../../');
const filesToBuild = ['workers/processChild', 'workers/threadChild', 'types'];
const filesToBuild = [
'workers/processChild',
'workers/threadChild',
'workers/withoutCircularRefs',
'types',
];
const writeDestination = join(__dirname, '__temp__');
const processChildWorkerPath = join(
writeDestination,
Expand Down
33 changes: 33 additions & 0 deletions packages/jest-worker/src/workers/__tests__/processChild.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,12 @@ beforeEach(() => {
mockCount++;

return {
fooCircularResult() {
const circular = {self: undefined as unknown};
circular.self = circular;
return {error: circular};
},

fooPromiseThrows() {
return new Promise((_resolve, reject) => {
setTimeout(() => reject(mockError), 5);
Expand Down Expand Up @@ -338,6 +344,33 @@ it('returns results when it gets resolved if function is asynchronous', async ()
expect(spyProcessSend).toHaveBeenCalledTimes(2);
});

it('returns results with circular references', () => {
process.emit(
'message',
[
CHILD_MESSAGE_INITIALIZE,
true, // Not really used here, but for type purity.
'./my-fancy-worker',
],
null,
);

process.emit(
'message',
[
CHILD_MESSAGE_CALL,
true, // Not really used here, but for type purity.
'fooCircularResult',
[],
],
null,
);

expect(spyProcessSend.mock.calls[0][0][1]).toEqual({
error: {self: expect.anything()},
});
});

it('calls the main module if the method call is "default"', () => {
process.emit(
'message',
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/**
* Copyright (c) Meta Platforms, Inc. and affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

import {withoutCircularRefs} from '../withoutCircularRefs';

it('test simple values', () => {
expect(withoutCircularRefs(undefined)).toBeUndefined();
expect(withoutCircularRefs(null)).toBeNull();
expect(withoutCircularRefs(0)).toBe(0);
expect(withoutCircularRefs('12')).toBe('12');
expect(withoutCircularRefs(true)).toBe(true);
expect(withoutCircularRefs([1])).toEqual([1]);
expect(withoutCircularRefs({a: 1, b: {c: 2}})).toEqual({a: 1, b: {c: 2}});
});

it('test circular values', () => {
const circular = {self: undefined as any};
circular.self = circular;

expect(withoutCircularRefs(circular)).toEqual({self: '[Circular]'});

expect(withoutCircularRefs([{a: circular, b: null}])).toEqual([
{a: {self: '[Circular]'}, b: null},
]);

expect(withoutCircularRefs({a: {b: circular}, c: undefined})).toEqual({
a: {b: {self: '[Circular]'}, c: undefined},
});
});
17 changes: 16 additions & 1 deletion packages/jest-worker/src/workers/messageParent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import {isMainThread, parentPort} from 'worker_threads';
import {PARENT_MESSAGE_CUSTOM} from '../types';
import {withoutCircularRefs} from './withoutCircularRefs';

export default function messageParent(
message: unknown,
Expand All @@ -15,7 +16,21 @@ export default function messageParent(
if (!isMainThread && parentPort != null) {
parentPort.postMessage([PARENT_MESSAGE_CUSTOM, message]);
} else if (typeof parentProcess.send === 'function') {
parentProcess.send([PARENT_MESSAGE_CUSTOM, message]);
try {
parentProcess.send([PARENT_MESSAGE_CUSTOM, message]);
} catch (error) {
if (error instanceof Error && /circular structure/.test(error?.message)) {
// We can safely send a message to the parent process again
// because previous sending was halted by "TypeError: Converting circular structure to JSON".
// But this time the message will be cleared from circular references.
parentProcess.send([
PARENT_MESSAGE_CUSTOM,
withoutCircularRefs(message),
]);
} else {
throw error;
}
}
} else {
throw new Error('"messageParent" can only be used inside a worker');
}
Expand Down
14 changes: 13 additions & 1 deletion packages/jest-worker/src/workers/processChild.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
PARENT_MESSAGE_SETUP_ERROR,
type ParentMessageMemUsage,
} from '../types';
import {withoutCircularRefs} from './withoutCircularRefs';

type UnknownFunction = (...args: Array<unknown>) => unknown | Promise<unknown>;

Expand Down Expand Up @@ -97,7 +98,18 @@ function reportSuccess(result: unknown) {
throw new Error('Child can only be used on a forked process');
}

process.send([PARENT_MESSAGE_OK, result]);
try {
process.send([PARENT_MESSAGE_OK, result]);
} catch (error) {
// We can safely send a message to the parent process again
// because previous sending was halted by "TypeError: Converting circular structure to JSON".
// But this time the message will be cleared from circular references.
if (error instanceof Error && /circular structure/.test(error?.message)) {
process.send([PARENT_MESSAGE_OK, withoutCircularRefs(result)]);
} else {
throw error;
}
}
}

function reportClientError(error: Error) {
Expand Down
27 changes: 27 additions & 0 deletions packages/jest-worker/src/workers/withoutCircularRefs.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/**
* Copyright (c) Meta Platforms, Inc. and affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

export function withoutCircularRefs(obj: unknown): unknown {
Copy link
Member

Choose a reason for hiding this comment

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

can we do the inverse as well - rebuild the object with the circular references in place?

I.e. what https://www.npmjs.com/package/flatted does (I see they now wanna push users to https://github.com/ungap/structured-clone/blob/main/README.md#extra-features, might be worth a look)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SimenB flatted and structured-clone have own serialization format, so both jest-runner and jest-worker should have the same serialize/parse logic. But currently runner and worker are different packages and end-users might have different versions on their installation.

So we break a transport layer by this improvement. Better to do it in another PR as improvement and R&D, this PR is a bug fix PR. And I'm very sorry but I don't have enough bandwidth to deal with bumping v30 😉

const cache = new WeakSet();
function copy(obj: unknown) {
if (typeof obj !== 'object' || obj === null) {
return obj;
}
if (cache.has(obj)) {
return '[Circular]';
}
cache.add(obj);
const copyObj: any = Array.isArray(obj) ? [] : {};
for (const key in obj) {
if (Object.prototype.hasOwnProperty.call(obj, key)) {
copyObj[key] = copy((obj as any)[key]);
}
}
return copyObj;
}
return copy(obj);
}