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 test failures with circular objects #10981

Merged
merged 8 commits into from
Dec 30, 2020

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Dec 28, 2020

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.

Copy link
Contributor

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

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.

packages/jest-worker/src/workers/ChildProcessWorker.ts Outdated Show resolved Hide resolved
const customMessageStarter = 'jest string - ';

export function serialize(data: unknown): unknown {
if (data != null && typeof data === 'object') {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

That said, I'm not against custom serializers

Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

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
Copy link
Member Author

SimenB commented Dec 28, 2020

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
Copy link
Member Author

SimenB commented Dec 28, 2020

@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
Copy link
Member Author

SimenB commented Dec 28, 2020

@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

Copy link
Contributor

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

@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.

CHANGELOG.md Outdated Show resolved Hide resolved
Comment on lines 31 to 43
it('test', () => {
const foo = {};
foo.ref = foo;

expect(foo).toEqual({});
});

it('test 2', () => {
const foo = {};
foo.ref = foo;

expect(foo).toEqual({});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

not needed, so removed it 👍

Co-authored-by: Micha Reiser <micha@famreiser.ch>
@SimenB SimenB changed the title fix(worker): handle circular messages fix(jest-runner): handle test failures with circular objects Dec 30, 2020
@SimenB SimenB merged commit 4c4162b into jestjs:master Dec 30, 2020
@SimenB SimenB deleted the circular-messages branch December 30, 2020 17:03
@ahnpnl
Copy link
Contributor

ahnpnl commented Jan 5, 2021

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

@a4lg
Copy link

a4lg commented Jan 24, 2021

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

@akamud
Copy link

akamud commented Feb 2, 2021

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

@SimenB
Copy link
Member Author

SimenB commented Feb 18, 2021

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

@SimenB
Copy link
Member Author

SimenB commented Mar 4, 2021

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
Copy link
Contributor

ahnpnl commented Mar 4, 2021

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

@SimenB
Copy link
Member Author

SimenB commented Mar 4, 2021

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
Copy link
Contributor

ahnpnl commented Mar 5, 2021

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
Copy link
Member Author

SimenB commented Mar 6, 2021

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

SimenB added a commit to SimenB/jest that referenced this pull request Mar 8, 2021
@SimenB
Copy link
Member Author

SimenB commented Mar 8, 2021

revert: #11172

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Circular references hang jest when assertions fail on node 14
6 participants