Serialize 'expected' and 'actual' in isolation with regards to already seen values

[malthe]

This fixes an issue in the test runner output:

const array = [1, 2, 3];
assert.deepEqual({array}, {array, other: 123});

If this assertion is made during a test, the "expected" output will be correct while the "actual" output will contain the string "Circular":

expected:
  array:
    0: 1
    1: 2
    2: 3
  other: 123
actual:
  array: <Circular>

With the change presented here, each output is produced in isolation from each other such that only circular references within each output will be shown as "Circular".

[nodejs-github-bot]

Review requested:

• @nodejs/test_runner

[aduh95]

Do we have tests for this so we validate how it actually affects the output?

[joesepi]

This should probably have a test case with an actual and expected that both share an object - like:
const o = { o: true }; const actual = { o }, expected = { o }

[malthe]

I have looked a bit around and it would seem that a test would go somewhere around here:

node/test/fixtures/test-runner/output/output.js

Line 379 in 281c342

test('assertion errors display actual and expected properly', async () => {

.

But I actually don't understand that existing test and I'm also a bit unsure about how to run the test using the binary I built at out/Release/node.

I tried using --test test/parallel/test-runner-output.mjs but this just gave a bunch of strange-looking errors.

[aduh95]

But I actually don't understand that existing test and I'm also a bit unsure about how to run the test using the binary I built at out/Release/node.

You can run it with the following command: python tools/test.py test/parallel/test-runner-output.mjs

[malthe]

Here's a test for this:

index f37d349503..95512e745f 100644
--- a/test/fixtures/test-runner/output/output.js
+++ b/test/fixtures/test-runner/output/output.js
@@ -382,8 +382,9 @@ test('assertion errors display actual and expected properly', async () => {
   circular.c = circular;
   const tmpLimit = Error.stackTraceLimit;
   Error.stackTraceLimit = 1;
+  const boo = [1];
   try {
-    assert.deepEqual({ foo: 1, bar: 1 }, circular); // eslint-disable-line no-restricted-properties
+    assert.deepEqual({ foo: 1, bar: 1, boo }, { boo, circular }); // eslint-disable-line no-restricted-properties
   } catch (err) {
     Error.stackTraceLimit = tmpLimit;
     throw err;

@aduh95 do you know what's the command that generates an updated snapshot file? I have tried --snapshot -j1 but the output is different.

[aduh95]

@malthe if you can't find the command in the error output, you can always update the snapshot manually.

[atlowChemi]

do you know what's the command that generates an updated snapshot file? I have tried --snapshot -j1 but the output is different.

try NODE_REGENERATE_SNAPSHOTS=1 ./node test/parallel/test-runner-output.mjs

node/test/common/README.md

Lines 686 to 690 in db7d22d

	### `NODE_REGENERATE_SNAPSHOTS`
	If set, test snapshots for a the current test are regenerated.
	for example `NODE_REGENERATE_SNAPSHOTS=1 out/Release/node test/parallel/test-runner-output.mjs`
	will update all the test runner output snapshots.

[malthe]

Test added (squashed with first commit).

[MoLow]

thanks for fixing this! can you amend the first commit message?

[malthe]

@MoLow squashed/amended now thanks.

[malthe]

bump

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/57452/

[malthe]

@MoLow not sure what these build failures are – they seem unrelated.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/57471/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/57483/

[malthe]

commits squashed

[MoLow]

@malthe can you also rebase ontop upstream/main?

[malthe]

@MoLow rebased

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/57489/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/57496/

[nodejs-github-bot]

Landed in 3ef7117