test_runner: use v8.serialize instead of TAP #47867

MoLow · 2023-05-04T21:14:24Z

Refs: #47663 (comment)
Fixes: #44656
Fixes: #47955
Fixes: #47481

not sure if this change is something we desire to document.

nodejs-github-bot · 2023-05-04T21:14:29Z

Review requested:

@nodejs/test_runner

lib/internal/test_runner/runner.js

lib/internal/test_runner/reporter/v8.js

cjihrig

Left some comments. The only thing I'm really concerned about is some of the changes in test/fixtures/test-runner/output/spec_reporter_cli.snapshot. You should also share the performance gains you shared with me on Slack 😄

lib/internal/test_runner/utils.js

lib/internal/error_serdes.js

lib/internal/test_runner/reporter/v8.js

lib/internal/test_runner/runner.js

test/fixtures/test-runner/output/spec_reporter_cli.snapshot

cjihrig

Requesting changes just so that this doesn't land yet now that there is an approval.

lib/internal/error_serdes.js

lib/internal/test_runner/reporter/v8.js

lib/internal/test_runner/runner.js

MoLow · 2023-05-05T05:24:48Z

The only thing I'm really concerned about is some of the changes in test/fixtures/test-runner/output/spec_reporter_cli.snapshot.

Well - this is kind of a good thing since most of the changes reduce the delta between running with and without --test, you can compare most changes with test/fixtures/test-runner/output/spec_reporter.snapshot.:
other changes are some edge cases that seem a little harder to handle.

(remember spec reporter includes each failing test twice when looking at this diff :))

diff --git a/test/fixtures/test-runner/output/spec_reporter_cli.js b/test/fixtures/test-runner/output/spec_reporter_cli.js
index e88a7221fb..6a7c2d655f 100644
--- a/test/fixtures/test-runner/output/spec_reporter_cli.js
+++ b/test/fixtures/test-runner/output/spec_reporter_cli.js
@@ -5,7 +5,7 @@ const fixtures = require('../../../common/fixtures');
 const spawn = require('node:child_process').spawn;
 
 const child = spawn(process.execPath,
-                    ['--no-warnings', '--test', '--test-reporter', 'spec', fixtures.path('test-runner/output/output.js')],
+                    ['--no-warnings', '--test-reporter', 'spec', fixtures.path('test-runner/output/output.js')],
                     { stdio: 'pipe' });
 // eslint-disable-next-line no-control-regex
 child.stdout.on('data', (d) => process.stdout.write(d.toString().replace(/[^\x00-\x7F]/g, '').replace(/\u001b\[\d+m/g, '')));
diff --git a/test/fixtures/test-runner/output/spec_reporter_cli.snapshot b/test/fixtures/test-runner/output/spec_reporter_cli.snapshot
index a8a628e1e1..7682041770 100644
--- a/test/fixtures/test-runner/output/spec_reporter_cli.snapshot
+++ b/test/fixtures/test-runner/output/spec_reporter_cli.snapshot
@@ -109,7 +109,7 @@
  subtest sync throw fail (*ms)
 
  sync throw non-error fail (*ms)
-  'Symbol(thrown symbol from sync throw non-error fail)'
+  Symbol(thrown symbol from sync throw non-error fail)
 
  level 0a
    level 1a (*ms)
@@ -185,8 +185,8 @@
   Error [ERR_TEST_FAILURE]: callback invoked multiple times
       * {
     failureType: 'multipleCallbackInvocations',
-    code: 'ERR_TEST_FAILURE',
-    [cause]: 'callback invoked multiple times'
+    cause: 'callback invoked multiple times',
+    code: 'ERR_TEST_FAILURE'
   }
 
  callback async throw (*ms)
@@ -206,10 +206,10 @@
 
  'only' and 'runOnly' require the --test-only command-line option.
  custom inspect symbol fail (*ms)
-  { foo: 1 }
+  customized
 
  custom inspect symbol that throws fail (*ms)
-  { foo: 1 }
+  { foo: 1, [Symbol(nodejs.util.inspect.custom)]: [Function: [nodejs.util.inspect.custom]] }
 
  subtest sync throw fails
    sync throw fails at first (*ms)
@@ -281,8 +281,8 @@
       * {
     generatedMessage: true,
     code: 'ERR_ASSERTION',
-    actual: { foo: 1, bar: 1 },
-    expected: <ref *1> { bar: 2, c: [Circular *1] },
+    actual: [Object],
+    expected: [Object],
     operator: 'deepEqual'
   }
 
@@ -400,7 +400,7 @@
       *
 
  sync throw non-error fail (*ms)
-  'Symbol(thrown symbol from sync throw non-error fail)'
+  Symbol(thrown symbol from sync throw non-error fail)
 
  +long running (*ms)
   'test did not finish before its parent and was cancelled'
@@ -440,8 +440,8 @@
   Error [ERR_TEST_FAILURE]: callback invoked multiple times
       * {
     failureType: 'multipleCallbackInvocations',
-    code: 'ERR_TEST_FAILURE',
-    [cause]: 'callback invoked multiple times'
+    cause: 'callback invoked multiple times',
+    code: 'ERR_TEST_FAILURE'
   }
 
  callback async throw (*ms)
@@ -450,10 +450,10 @@
       *
 
  custom inspect symbol fail (*ms)
-  { foo: 1 }
+  customized
 
  custom inspect symbol that throws fail (*ms)
-  { foo: 1 }
+  { foo: 1, [Symbol(nodejs.util.inspect.custom)]: [Function: [nodejs.util.inspect.custom]] }
 
  sync throw fails at first (*ms)
   Error: thrown from subtest sync throw fails at first
@@ -519,8 +519,8 @@
       * {
     generatedMessage: true,
     code: 'ERR_ASSERTION',
-    actual: { foo: 1, bar: 1 },
-    expected: <ref *1> { bar: 2, c: [Circular *1] },
+    actual: [Object],
+    expected: [Object],
     operator: 'deepEqual'
   }

You should also share the performance gains you shared with me on Slack 😄

Sure! I tried later on and it seemed like there is not always such a good performance gain - it really depends on the suite you run.
for example, running NODE_REGENERATE_SNAPSHOTS=1 ./node test/parallel/test-runner-output.mjs with and without this change perform pretty much the same.
however, I did a comparison on some tests suite I run at work and the results were:

v18.16.0:    ℹ duration_ms 22166.480125
v20.1.0:     ℹ duration_ms 6148.818709
v21.0.0-pre: ℹ duration_ms 5225.974167

MoLow · 2023-05-05T11:11:59Z

now there almost no delta between with and without --test, besides some very specific edge cases

cjihrig · 2023-05-05T13:01:42Z

Well - this is kind of a good thing since most of the changes reduce the delta between running with and without --test

Thanks for clarifying. That's good news.

cjihrig

The changes in lib/internal/error_serdes.js should be in a separate commit, and arguably a separate PR, and with approval from @nodejs/workers

lib/internal/test_runner/utils.js

lib/internal/test_runner/reporter/v8.js

cjihrig

The changes in lib/internal/error_serdes.js should be in a separate commit, and arguably a separate PR, and with approval from @nodejs/workers

lib/internal/error_serdes.js

lib/internal/test_runner/reporter/v8.js

ljharb · 2023-05-08T15:22:17Z

lib/internal/test_runner/runner.js

+
+    // Index 0 should always be present because we just pushed data into it.
+    let messageBufferHead = this.#messageBuffer[0];
+    let headerIndex = messageBufferHead.indexOf(v8Header);


Suggested change

let headerIndex = messageBufferHead.indexOf(v8Header);

let headerIndex = ArrayPrototypeIndexOf(messageBufferHead, v8Header);

messageBufferHead is a Buffer

If it's a Buffer, you should be able to use Uint8ArrayPrototypeIndexOf

I have tried but:

./node --expose-internals -p "require('internal/test/binding').primordials.Uint8ArrayPrototypeIndexOf()"

results in require(...).primordials.Uint8ArrayPrototypeIndexOf is not a function
and

./node --expose-internals -p "require('internal/test/binding').primordials.TypedArrayPrototypeIndexOf(Buffer.from('Hello'), Buffer.from('ll'))"

results in -1

lib/internal/test_runner/runner.js

nodejs-github-bot · 2023-05-15T09:46:30Z

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

cjihrig · 2023-05-15T14:07:05Z

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

+    process.stdout.write(Buffer.from("arbitrary - mid"));
+    process.stdout.write(Buffer.concat([chunk, Buffer.from("arbitrary - post")]));
+  }
+})();


Should we have a .then(common.mustCall()) here?

no need, we snapshot the output

cjihrig · 2023-05-15T14:07:48Z

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

+// Flags: --test --expose-internals
+'use strict';
+
+const v8_reporter = require('internal/test_runner/reporter/v8-serializer');


Suggested change

const v8_reporter = require('internal/test_runner/reporter/v8-serializer');

const v8Reporter = require('internal/test_runner/reporter/v8-serializer');

if this is the only comment, Il skip it :)

nodejs-github-bot · 2023-05-15T14:27:47Z

Landed in 7116bc0

PR-URL: #47867 Fixes: #44656 Fixes: #47955 Fixes: #47481 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

MoLow · 2023-05-23T05:57:51Z

note: if backported, please backport along with #48106

PR-URL: #47867 Fixes: #44656 Fixes: #47955 Fixes: #47481 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

PR-URL: nodejs#47867 Fixes: nodejs#44656 Fixes: nodejs#47955 Fixes: nodejs#47481 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

MoLow requested a review from cjihrig May 4, 2023 21:14

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels May 4, 2023

MoLow commented May 4, 2023

View reviewed changes

lib/internal/test_runner/runner.js Outdated Show resolved Hide resolved

anonrig reviewed May 4, 2023

View reviewed changes

lib/internal/test_runner/reporter/v8.js Outdated Show resolved Hide resolved

anonrig approved these changes May 4, 2023

View reviewed changes

cjihrig reviewed May 4, 2023

View reviewed changes

cjihrig requested changes May 4, 2023

View reviewed changes

aduh95 reviewed May 4, 2023

View reviewed changes

lib/internal/error_serdes.js Outdated Show resolved Hide resolved

ljharb reviewed May 5, 2023

View reviewed changes

lib/internal/error_serdes.js Outdated Show resolved Hide resolved

lib/internal/test_runner/reporter/v8.js Outdated Show resolved Hide resolved

lib/internal/test_runner/runner.js Show resolved Hide resolved

lib/internal/test_runner/runner.js Outdated Show resolved Hide resolved

MoLow force-pushed the test-runner-v8-reporter branch 2 times, most recently from d9c12f8 to cb71d5c Compare May 5, 2023 11:10

benjamingr approved these changes May 6, 2023

View reviewed changes

MoLow force-pushed the test-runner-v8-reporter branch from cb71d5c to 4f92f53 Compare May 7, 2023 14:06

MoLow requested a review from cjihrig May 8, 2023 11:36

MoLow force-pushed the test-runner-v8-reporter branch 2 times, most recently from b0698ed to 7990e81 Compare May 8, 2023 13:22

cjihrig reviewed May 8, 2023

View reviewed changes

lib/internal/test_runner/utils.js Show resolved Hide resolved

lib/internal/test_runner/reporter/v8.js Outdated Show resolved Hide resolved

cjihrig reviewed May 8, 2023

View reviewed changes

ljharb reviewed May 8, 2023

View reviewed changes

MoLow mentioned this pull request May 8, 2023

worker: support more cases when (de)serializing errors #47925

Merged

cjihrig mentioned this pull request May 10, 2023

Test runner does not print Error cause #44656

Closed

cjihrig linked an issue May 10, 2023 that may be closed by this pull request

Test runner does not print Error cause #44656

Closed

This was referenced May 10, 2023

test: fix test runner does not print error cause #46576

Closed

test_runner: it.skip and it.todo are not working as expected in the reporter #47481

Closed

MoLow added the test_runner Issues and PRs related to the test runner subsystem. label May 11, 2023

This comment was marked as outdated.

Sign in to view

MoLow added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 15, 2023

This comment was marked as outdated.

Sign in to view

MoLow requested a review from cjihrig May 15, 2023 10:29

MoLow mentioned this pull request May 15, 2023

Test Runner: delegate arbitrary output formatting to each reporter #48011

Closed

cjihrig approved these changes May 15, 2023

View reviewed changes

MoLow added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels May 15, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 15, 2023

nodejs-github-bot merged commit 7116bc0 into nodejs:main May 15, 2023

targos mentioned this pull request May 15, 2023

v20.2.0 release proposal #48020

Merged

MoLow deleted the test-runner-v8-reporter branch May 23, 2023 05:57

cjihrig mentioned this pull request Jun 8, 2023

TAP parser fails with zero width joiner #48397

Closed

danielleadams mentioned this pull request Jul 10, 2023

v18.17.0 release proposal #48694

Merged

MoLow mentioned this pull request Jul 24, 2023

.cause of errors innode:test test cases not available to custom reporter #48900

Closed

voxpelli mentioned this pull request Jul 25, 2023

chai.should() breaks error serialization #48918

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test_runner: use v8.serialize instead of TAP #47867

test_runner: use v8.serialize instead of TAP #47867

MoLow commented May 4, 2023 •

edited

Loading

nodejs-github-bot commented May 4, 2023

cjihrig left a comment

cjihrig left a comment

MoLow commented May 5, 2023

MoLow commented May 5, 2023

cjihrig commented May 5, 2023

cjihrig left a comment

cjihrig left a comment

ljharb May 8, 2023

MoLow May 14, 2023

targos May 14, 2023

MoLow May 14, 2023 •

edited

Loading

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented May 15, 2023

cjihrig May 15, 2023

MoLow May 15, 2023

cjihrig May 15, 2023

MoLow May 15, 2023

nodejs-github-bot commented May 15, 2023

MoLow commented May 23, 2023

	let headerIndex = messageBufferHead.indexOf(v8Header);
	let headerIndex = ArrayPrototypeIndexOf(messageBufferHead, v8Header);

	const v8_reporter = require('internal/test_runner/reporter/v8-serializer');
	const v8Reporter = require('internal/test_runner/reporter/v8-serializer');

test_runner: use v8.serialize instead of TAP #47867

test_runner: use v8.serialize instead of TAP #47867

Conversation

MoLow commented May 4, 2023 • edited Loading

nodejs-github-bot commented May 4, 2023

cjihrig left a comment

Choose a reason for hiding this comment

cjihrig left a comment

Choose a reason for hiding this comment

MoLow commented May 5, 2023

MoLow commented May 5, 2023

cjihrig commented May 5, 2023

cjihrig left a comment

Choose a reason for hiding this comment

cjihrig left a comment

Choose a reason for hiding this comment

ljharb May 8, 2023

Choose a reason for hiding this comment

MoLow May 14, 2023

Choose a reason for hiding this comment

targos May 14, 2023

Choose a reason for hiding this comment

MoLow May 14, 2023 • edited Loading

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented May 15, 2023

cjihrig May 15, 2023

Choose a reason for hiding this comment

MoLow May 15, 2023

Choose a reason for hiding this comment

cjihrig May 15, 2023

Choose a reason for hiding this comment

MoLow May 15, 2023

Choose a reason for hiding this comment

nodejs-github-bot commented May 15, 2023

MoLow commented May 23, 2023

MoLow commented May 4, 2023 •

edited

Loading

MoLow May 14, 2023 •

edited

Loading