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

SimenB · 2020-12-28T13:48:21Z

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.

packages/jest-worker/src/workers/messageParent.ts

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.

packages/jest-worker/src/workers/ChildProcessWorker.ts

MichaReiser · 2020-12-28T14:27:13Z

packages/jest-worker/src/workers/utils.ts

+const customMessageStarter = 'jest string - ';
+
+export function serialize(data: unknown): unknown {
+  if (data != null && typeof data === 'object') {


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.

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

That said, I'm not against custom serializers

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 · 2020-12-28T15:49:37Z

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 · 2020-12-28T17:18:34Z

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 · 2020-12-28T19:14:08Z

@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 · 2020-12-28T19:37:31Z

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

CHANGELOG.md

MichaReiser · 2020-12-29T08:53:12Z

e2e/__tests__/circularInequality.test.ts

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


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

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

not needed, so removed it 👍

Co-authored-by: Micha Reiser <[email protected]>

ahnpnl · 2021-01-05T00:32:17Z

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

a4lg · 2021-01-24T12:42:58Z

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

akamud · 2021-02-02T02:09:25Z

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

SimenB · 2021-02-18T22:13:28Z

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

SimenB · 2021-03-04T07:38:08Z

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 · 2021-03-04T12:02:29Z

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

SimenB · 2021-03-04T13:51:13Z

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 · 2021-03-05T10:23:04Z

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 · 2021-03-06T08:48:48Z

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

…estjs#10981)" This reverts commit 4c4162b.

SimenB · 2021-03-08T12:15:19Z

revert: #11172

…10981) (#11172)

github-actions · 2021-05-10T22:06:34Z

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.

SimenB requested a review from MichaReiser December 28, 2020 13:48

facebook-github-bot added the cla signed label Dec 28, 2020

SimenB commented Dec 28, 2020

View reviewed changes

packages/jest-worker/src/workers/messageParent.ts Outdated Show resolved Hide resolved

SimenB commented Dec 28, 2020

View reviewed changes

packages/jest-worker/src/workers/messageParent.ts Outdated Show resolved Hide resolved

SimenB requested review from jeysal and thymikee December 28, 2020 14:00

SimenB mentioned this pull request Dec 28, 2020

fix(jest-worker): Remove circular references from messages sent to workers #10881

Closed

MichaReiser reviewed Dec 28, 2020

View reviewed changes

This was referenced Dec 28, 2020

fix(workers): use advanced serialization by default in child process workers #10983

Merged

fix(worker): do not swallow errors when sending custom messages #10984

Merged

SimenB force-pushed the circular-messages branch from 09d057d to 11d6d0a Compare December 28, 2020 15:39

SimenB force-pushed the circular-messages branch from 11d6d0a to 01ab943 Compare December 28, 2020 17:17

SimenB force-pushed the circular-messages branch from da0ff91 to 8dd1bba Compare December 28, 2020 19:35

MichaReiser approved these changes Dec 29, 2020

View reviewed changes

SimenB mentioned this pull request Dec 30, 2020

fix(expect): resolve message on assertion errors #10989

Merged

SimenB added 3 commits December 30, 2020 17:15

fix(worker): handle circular messages

5e8ae67

revert worker changes

6e282ca

fix: use advanced serialization when spawning jest workers

ff90e08

SimenB force-pushed the circular-messages branch from 8dd1bba to ff90e08 Compare December 30, 2020 16:15

Update CHANGELOG.md

ca79177

Co-authored-by: Micha Reiser <[email protected]>

SimenB changed the title ~~fix(worker): handle circular messages~~ fix(jest-runner): handle test failures with circular objects Dec 30, 2020

SimenB added 4 commits December 30, 2020 17:32

Update index.ts

049042d

Update CHANGELOG.md

ae8691b

no need for duplicate tests

c405e0f

less syntax

5325860

SimenB merged commit 4c4162b into jestjs:master Dec 30, 2020

SimenB deleted the circular-messages branch December 30, 2020 17:03

calebeby mentioned this pull request Jan 26, 2021

"messageParent" can only be used inside a worker cloudfour/pleasantest#12

Closed

SimenB added a commit to SimenB/jest that referenced this pull request Mar 8, 2021

Revert "fix(jest-runner): handle test failures with circular objects (j…

123a18b

…estjs#10981)" This reverts commit 4c4162b.

SimenB added a commit that referenced this pull request Mar 8, 2021

Revert "fix(jest-runner): handle test failures with circular objects (#…

e876ae0

…10981) (#11172)

dependabot bot mentioned this pull request Mar 12, 2021

npm: bump jest from 27.0.0-next.2 to 27.0.0-next.4 AlexRogalskiy/github-action-proverbs#25

Closed

github-actions bot locked as resolved and limited conversation to collaborators May 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

SimenB commented Dec 28, 2020 •

edited

Loading

MichaReiser left a comment

MichaReiser Dec 28, 2020

SimenB Dec 28, 2020

MichaReiser Dec 28, 2020

MichaReiser commented Dec 28, 2020

SimenB commented Dec 28, 2020

SimenB commented Dec 28, 2020

SimenB commented Dec 28, 2020

MichaReiser left a comment

MichaReiser Dec 29, 2020

SimenB Dec 29, 2020

SimenB Dec 30, 2020

ahnpnl commented Jan 5, 2021 •

edited

Loading

a4lg commented Jan 24, 2021

akamud commented Feb 2, 2021

SimenB commented Feb 18, 2021

SimenB commented Mar 4, 2021 •

edited

Loading

ahnpnl commented Mar 4, 2021

SimenB commented Mar 4, 2021

ahnpnl commented Mar 5, 2021 •

edited

Loading

SimenB commented Mar 6, 2021

SimenB commented Mar 8, 2021

github-actions bot commented May 10, 2021

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

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

Conversation

SimenB commented Dec 28, 2020 • edited Loading

Summary

Test plan

MichaReiser left a comment

Choose a reason for hiding this comment

MichaReiser Dec 28, 2020

Choose a reason for hiding this comment

SimenB Dec 28, 2020

Choose a reason for hiding this comment

MichaReiser Dec 28, 2020

Choose a reason for hiding this comment

MichaReiser commented Dec 28, 2020

SimenB commented Dec 28, 2020

SimenB commented Dec 28, 2020

SimenB commented Dec 28, 2020

MichaReiser left a comment

Choose a reason for hiding this comment

MichaReiser Dec 29, 2020

Choose a reason for hiding this comment

SimenB Dec 29, 2020

Choose a reason for hiding this comment

SimenB Dec 30, 2020

Choose a reason for hiding this comment

ahnpnl commented Jan 5, 2021 • edited Loading

a4lg commented Jan 24, 2021

akamud commented Feb 2, 2021

SimenB commented Feb 18, 2021

SimenB commented Mar 4, 2021 • edited Loading

ahnpnl commented Mar 4, 2021

SimenB commented Mar 4, 2021

ahnpnl commented Mar 5, 2021 • edited Loading

SimenB commented Mar 6, 2021

SimenB commented Mar 8, 2021

github-actions bot commented May 10, 2021

SimenB commented Dec 28, 2020 •

edited

Loading

ahnpnl commented Jan 5, 2021 •

edited

Loading

SimenB commented Mar 4, 2021 •

edited

Loading

ahnpnl commented Mar 5, 2021 •

edited

Loading