fix(jest-runner): handle test failures with circular objects

[SimenB]

Summary

Structured clone doesn't handle circular objects seemingly, so I've added telejson which serializes it into a special string that it then restores.

It might make sense to make the serializer pluggable?

Uses advanced serialization for Jest's test worker, and ensuring the assertion error we use can be serialized.

Fixes #10577
Closes #10881

Test plan

E2E test added.

[MichaReiser]

The code looks code to me. My main concerns still are

• Additional overhead payed by all workers that do not need structured clone nor have cyclic dependencies
• Cyclic dependencies are supported by the structured clone algorithm. E.g. the following works fine if I changed the serialization to advanced or use worker threads.

main.js

const JestWorker = require('../build').Worker;

const farm = new JestWorker(require.resolve('./cyclic-worker'), {
    exposedMethods: ['identity'],
    numWorkers: 2,
});

async function test() {
    const data = {a: 2, b: undefined};
    data.b = data;

    console.log(await farm.identity(data));

    farm.end();
}

test().then((res, error) => {
    if (error) {
        console.error(error);
    }
});

worker.js

module.exports = {
    identity(a) {
        return a;
    }
}

Output:

~/g/j/p/jest-worker [master]× » node src/cyclic-dep.test.js                                                                                                                 -1- 14:37:27
<ref *1> { a: 2, b: [Circular *1] }

I guess the main issue is that the passed object makes use of any non-cloneable objects like Symbols / Proxies.

[MichaReiser]

Isn't e.g. message always an object? That means all jest-worker clients will always have to pay for the telejson overhead. That might be a significant penalty for larger repositories that use jest-hastemap or Metro where the function is called for every file in the repository/app.

I think it would be worth to avoid this overhead by introducing a new option that allows customizing the serialization either

• by jest-worker providing a predefined set of serialisation formats (easier)
• requiring a path to a mdoule that exports serialize/`deserialize.

[SimenB]

no, it's whatever the user passes, e.g. strings, numbers etc.

That said, I'm not against custom serializers

[MichaReiser]

I see. That is because the messages from parent to child don't use the custom serialization? I think we should make sure that the setupArgs in the INIT message and the arguments passed in a method call are correctly serialized as well.

[MichaReiser]

The code looks good to me but I think we should investigate why the structured cloning isn't sufficient. It isn't because of the circular references as my test proofed. The use of telejson is hidding the underlying issue.

[SimenB]

The code looks good to me but I think we should investigate why the structured cloning isn't sufficient. It isn't because of the circular references as my test proofed. The use of telejson is hidding the underlying issue.

Agreed, I'll investigate at some point

[SimenB]

@MichaReiser I dug a bit more into this, and the issue is that we attempt to send a function over that closes over a variable out of scope. Use this as the worker and it fails:

module.exports = {
  identity() {
    const thing = 'foobar';

    return () => thing;
  },
};

Although it fails with Call retries were exceeded rather than the underlying serialization error.

Concretely in Jest, the issue is the message function we add to errors. This diff "fixes" it..

diff --git i/packages/expect/src/index.ts w/packages/expect/src/index.ts
index e050afe40a..1c6cc45260 100644
--- i/packages/expect/src/index.ts
+++ w/packages/expect/src/index.ts
@@ -293,7 +293,7 @@ const makeThrowingMatcher = (
         // Passing the result of the matcher with the error so that a custom
         // reporter could access the actual and expected objects of the result
         // for example in order to display a custom visual diff
-        error.matcherResult = result;
+        error.matcherResult = {...result, message};
 
         if (throws) {
           throw error;
diff --git i/packages/jest-worker/src/workers/ChildProcessWorker.ts w/packages/jest-worker/src/workers/ChildProcessWorker.ts
index 6b7daa4758..33b57a1457 100644
--- i/packages/jest-worker/src/workers/ChildProcessWorker.ts
+++ w/packages/jest-worker/src/workers/ChildProcessWorker.ts
@@ -92,6 +92,9 @@ export default class ChildProcessWorker implements WorkerInterface {
       } as NodeJS.ProcessEnv,
       // Suppress --debug / --inspect flags while preserving others (like --harmony).
       execArgv: process.execArgv.filter(v => !/^--(debug|inspect)/.test(v)),
+      // default to advanced serialization in order to match worker threads
+      // @ts-expect-error: option does not exist on the node 10 types
+      serialization: 'advanced',
       silent: true,
       ...this._options.forkOptions,
     });

So serialization: 'advanced' fixes the circularity error, but introduces a new one (to match worker threads)

[SimenB]

@MichaReiser I pushed up a change using advanced serialization in jest-runner instead of changing the default pending #10983. Will probably need to skip the test on node 10

[MichaReiser]

@MichaReiser I pushed up a change using advanced serialization in jest-runner instead of changing the default pending #10983. Will probably need to skip the test on node 10

That makes sense and thanks for figuring out the issue. Not changing the default could make sense as the semantics are quite different as we learned. It's unfortunate that Node doesn't provide a more useful error message. Must have been painful to identify the source.

Will probably need to skip the test on node 10

That's probably fine. An alternative would be to use telejson in jest-worker if the serialization format is advanced and we're running Node 10 to mimic structured clone. But I'm not sure if it's worth the effort as Node 10 is EOL in April next year.

[MichaReiser]

Is there anything different between the two tests that I'm overlooking or are they the same?

[SimenB]

They're identical. Might be enough to just have one, haven't verified

[SimenB]

not needed, so removed it 👍

[ahnpnl]

Hi @SimenB would you please release this fix in a new next tag ? I'd love to test it.

[a4lg]

Not only it resolved the issue on circular objects, it did resolved the problem I encountered while testing with BigInt. Thanks!

[akamud]

Any idea when this will be available in a npm release?

[SimenB]

v27.0.0-next.3 is released now, sorry about the delay

[SimenB]

I will have to revert this as any errors thrown in tests containing stuff that cannot be serialized (like a function closing av an out-of-scope variable) will make the test runner crash.

const someString = 'hello from the other side';

test('dummy', () => {
  const error = new Error('boom');

  error.someProp = () => someString;

  throw error;
});

Running this test on its own is fine, but having 2 of them and running in test mode leads to

Error: () => someString could not be cloned.
    at writeChannelMessage (node:internal/child_process/serialization:81:9)
    at process.target._send (node:internal/child_process:822:17)
    at process.target.send (node:internal/child_process:722:19)
    at reportSuccess (/Users/simen/repos/jest/packages/jest-worker/build/workers/processChild.js:67:11)
node:internal/child_process/serialization:81
    ser.writeValue(message);

We cannot make assumptions about how errors thrown (or anything else used to fail tests) are structured

[ahnpnl]

Out of curiosity, is worker serialization advanced similar to V8 serialize function?

[SimenB]

yep. https://nodejs.org/api/child_process.html#child_process_advanced_serialization

Child processes support a serialization mechanism for IPC that is based on the serialization API of the v8 module, based on the HTML structured clone algorithm.

[ahnpnl]

Oh i got the similar error like the one you mentioned above

internal/child_process/serialization.js:69
      deserializer.readHeader();
                   ^

Error: Unable to deserialize cloned data due to invalid or unsupported version.
    at ChildProcessDeserializer.readHeader (<anonymous>)
    at parseChannelMessages (internal/child_process/serialization.js:69:20)
    at parseChannelMessages.next (<anonymous>)
    at Pipe.channel.onread (internal/child_process.js:592:18)

Even though it happened randomly.

[SimenB]

Yep, that's caused by this PR and why I need to revert it

[SimenB]

revert: #11172

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.