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-runner): Handle unsafe worker_threads structureClone with function type in matchers #14436

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
34 changes: 34 additions & 0 deletions packages/jest-runner/src/__tests__/helpers.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/**
* 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 {replaceFunctionsWithStringReferences} from '../helpers';

it('serialize functions inside the nested object', () => {
const obj = {
foo: () => {},
nested: {
fn: function bar() {
return 0;
},
},
nestedArray: [{baz: function baz() {}}, () => {}],
};

expect(replaceFunctionsWithStringReferences(obj)).toEqual({
foo: '[Function foo]',
nested: {
fn: '[Function bar]',
},
nestedArray: [
{
baz: '[Function baz]',
},
'[Function anonymous]',
],
});
});
29 changes: 29 additions & 0 deletions packages/jest-runner/src/helpers.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/**
* 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 const replaceFunctionsWithStringReferences = <T>(value: T): T => {
if (typeof value === 'function') {
return `[Function ${value.name || 'anonymous'}]` as unknown as T;
}

if (Array.isArray(value)) {
return value.map(replaceFunctionsWithStringReferences) as T;
}

const isObject = value !== null && typeof value === 'object';
if (isObject) {
const oldObject = value as Record<string, unknown>;
const newObject: Record<string, unknown> = {};
for (const key of Object.keys(value)) {
newObject[key] = replaceFunctionsWithStringReferences(oldObject[key]);
}

return newObject as T;
}

return value;
};
24 changes: 23 additions & 1 deletion packages/jest-runner/src/testWorker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {separateMessageFromStack} from 'jest-message-util';
import type Resolver from 'jest-resolve';
import Runtime from 'jest-runtime';
import {messageParent} from 'jest-worker';
import {replaceFunctionsWithStringReferences} from './helpers';
import runTest from './runTest';
import type {ErrorWithCode, TestRunnerSerializedContext} from './types';

Expand Down Expand Up @@ -96,7 +97,7 @@ export async function worker({
context,
}: WorkerData): Promise<TestResult> {
try {
return await runTest(
const testResult = await runTest(
path,
globalConfig,
config,
Expand All @@ -110,7 +111,28 @@ export async function worker({
},
sendMessageToJest,
);
return makeSerializableTestResults(testResult);
} catch (error: any) {
throw formatError(error);
}
}

function makeSerializableTestResults(result: TestResult): TestResult {
const {testResults} = result;
const serializableResults = testResults.map(resultItem => {
if (resultItem.failureDetails) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do I get it right: the problem is that failureDetails includes data which cannot be serialised? If so, it be better to serialised it earlier in the test runner.

This was discussed already, I will try to dig out the thread. The idea is that we know in advance that all what the runner returns has to be serialisable, because that data will be passed through workers. If I remember it right, the test runner is already serialising most of the data. Seem like something was simply overlooked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do I get it right: the problem is that failureDetails includes data which cannot be serialised?

Yes, exactly, the problem with function type which throw an error during clone. Also DOM nodes might cause similar behaviour, but I'm not sure if it's a valid case for workers.

If so, it be better to serialised it earlier in the test runner.

I was also thinking about it, my only concern was that it might be exposed to other tools (outside of jest repo). But if the approach is ok I can fix this and add some tests to cover the serialisation

This was discussed already, I will try to dig out the thread

Sounds good, found this related PR, would be nice to look into other discussions

Copy link
Contributor

Choose a reason for hiding this comment

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

Here it is. I was talking about #11467. The conclusions are made in #11467 (comment) and following comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mrazauskas Thanks a lot, now I have full context

return {
...resultItem,
// Functions cause DataCloneError when passed between workers,
// therefore they are converted to string references
failureDetails: replaceFunctionsWithStringReferences(
resultItem.failureDetails,
),
};
}

return resultItem;
});

return {...result, testResults: serializableResults};
}
Loading