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

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

Merged
merged 1 commit into from
Feb 29, 2024

Conversation

malthe
Copy link
Contributor

@malthe malthe commented Feb 23, 2024

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

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Feb 23, 2024
@aduh95
Copy link
Contributor

aduh95 commented Feb 23, 2024

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

@joesepi
Copy link
Member

joesepi commented Feb 23, 2024

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

malthe commented Feb 23, 2024

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

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

aduh95 commented Feb 23, 2024

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

malthe commented Feb 24, 2024

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

aduh95 commented Feb 24, 2024

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

@atlowChemi
Copy link
Member

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

malthe commented Feb 25, 2024

Test added (squashed with first commit).

@MoLow
Copy link
Member

MoLow commented Feb 25, 2024

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

@malthe malthe force-pushed the reporter-tap-avoid-circular branch from 8ea3657 to 5c42ba6 Compare February 25, 2024 14:00
@malthe
Copy link
Contributor Author

malthe commented Feb 25, 2024

@MoLow squashed/amended now thanks.

@malthe malthe force-pushed the reporter-tap-avoid-circular branch from 5c42ba6 to e29c56e Compare February 25, 2024 14:18
@malthe
Copy link
Contributor Author

malthe commented Feb 27, 2024

bump

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Feb 27, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 27, 2024
@nodejs-github-bot
Copy link
Collaborator

@malthe
Copy link
Contributor Author

malthe commented Feb 27, 2024

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

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@malthe malthe force-pushed the reporter-tap-avoid-circular branch from 352a556 to 628e39a Compare February 28, 2024 11:34
@malthe
Copy link
Contributor Author

malthe commented Feb 28, 2024

commits squashed

@MoLow
Copy link
Member

MoLow commented Feb 28, 2024

@malthe can you also rebase ontop upstream/main?

Previously, a value seen in 'actual' would be serialized as a circular
reference if it had also appeared in 'expected'.
@malthe malthe force-pushed the reporter-tap-avoid-circular branch from 628e39a to e6b5a64 Compare February 28, 2024 11:54
@malthe
Copy link
Contributor Author

malthe commented Feb 28, 2024

@MoLow rebased

@MoLow MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 28, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 28, 2024
@MoLow MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 28, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 28, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@MoLow MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 29, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 29, 2024
@nodejs-github-bot nodejs-github-bot merged commit 3ef7117 into nodejs:main Feb 29, 2024
58 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 3ef7117

marco-ippolito pushed a commit that referenced this pull request Feb 29, 2024
Previously, a value seen in 'actual' would be serialized as a circular
reference if it had also appeared in 'expected'.

PR-URL: #51851
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
marco-ippolito pushed a commit that referenced this pull request Feb 29, 2024
Previously, a value seen in 'actual' would be serialized as a circular
reference if it had also appeared in 'expected'.

PR-URL: #51851
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
@marco-ippolito marco-ippolito mentioned this pull request Mar 1, 2024
richardlau pushed a commit that referenced this pull request Mar 25, 2024
Previously, a value seen in 'actual' would be serialized as a circular
reference if it had also appeared in 'expected'.

PR-URL: #51851
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
richardlau pushed a commit that referenced this pull request Mar 25, 2024
Previously, a value seen in 'actual' would be serialized as a circular
reference if it had also appeared in 'expected'.

PR-URL: #51851
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
@richardlau richardlau mentioned this pull request Mar 25, 2024
rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Mar 26, 2024
Previously, a value seen in 'actual' would be serialized as a circular
reference if it had also appeared in 'expected'.

PR-URL: nodejs#51851
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants