test_runner: add reporters#45712

MoLow · 2022-12-02T07:41:07Z

TODO:

docs
tests
support run api?

lib/internal/test_runner/test.js

cjihrig

Left some comments. I have some concerns about the approach here. I know this is still a WIP, but I also think we should wait to merge this until we at least have a concrete plan for multiple reporters and destinations, just so that we don't paint ourselves into a corner.

src/node_options.cc

lib/internal/main/test_runner.js

lib/internal/test_runner/runner.js

lib/internal/test_runner/utils.js

lib/internal/test_runner/tap_stream.js

MoLow · 2022-12-03T17:16:54Z

. I have some concerns about the approach here.

can you elaborate? what alternative approach did you have in mind?

I know this is still a WIP, but I also think we should wait to merge this until we at least have a concrete plan for multiple reporters and destinations, just so that we don't paint ourselves into a corner.

👍🏻 I think the main challenge is a DX challenge, not a technical challenge - so lets continue the brainstorm in #45648 before merging this

cjihrig · 2022-12-03T17:46:28Z

can you elaborate? what alternative approach did you have in mind?

I just meant the things that I left comments on. However, the approach here is making me rethink some things. If we aren't going to build reporters on top of the existing TAP work, then maybe we should not default to TAP everywhere and just make that the default reporter.

MoLow · 2022-12-03T18:03:59Z

If we aren't going to build reporters on top of the existing TAP work, then maybe we should not default to TAP everywhere and just make that the default reporter.

that is pretty much what I have suggested here, maybe I have not described it well
#43344 (comment)

lib/internal/test_runner/reporter_stream.js

lib/internal/test_runner/utils.js

lib/internal/main/test_runner.js

lib/internal/test_runner/tap_stream.js

lib/internal/test_runner/test.js

src/node_options.h

lib/test/reporter/dot.js

cjihrig · 2022-12-07T20:45:10Z

👍🏻 I think the main challenge is a DX challenge, not a technical challenge - so lets continue the brainstorm in #45648 before merging this

The more I think about this, the more I like the idea of having a CLI flag for destinations, and if you use that flag, there have to be an equal number of reporters and destinations, and reporter 1 gets paired with destination 1, and so on.

MoLow · 2022-12-09T12:59:51Z

@cjihrig pushed another iteration implementing the discussed here,
this now supports multiple reporters,
would love some feedback

cjihrig

Left a handful of comments. I think this is heading in a good direction though. It looks like there are some relevant CI failures as well.

lib/test/reporter/spec.js

lib/test/reporter/tap.js

lib/internal/test_runner/utils.js

lib/test/reporter/tap.js

cjihrig · 2022-12-09T18:42:20Z

lib/test/reporter/tap.js

+yieldreportTest(data.nesting,data.testNumber,'not ok',data.name,data.directive);
+yieldreportDetails(data.nesting,data.details);


Should we be yielding twice here? Same question for 'test:pass'.

just kept the same behavior as before, we sent this as two different chunks

lib/test/reporter/tap.js

MoLow · 2022-12-16T00:37:37Z

@nodejs/test_runner I think this is ready for review

lib/internal/modules/utils.js

ljharb · 2022-12-16T05:42:13Z

lib/internal/modules/utils.js

+const{ pathToFileURL }=require('internal/url');
+const{ isAbsolute }=require('path');
+constfile=isAbsolute(filePath) ? pathToFileURL(filePath).href : filePath;
+returnesmLoader.import(file,undefined,ObjectCreate(null));


Suggested change
returnesmLoader.import(file,undefined,ObjectCreate(null));
returnesmLoader.import(file,undefined,{__proto__: null});

I wish we could have just 1 way to do this—I don't care which of the 3–4 I'm aware of.

I don't understand why the proto approach - undeniable syntax that doesn't require a primordial or a function call - wouldn't be preferred, but either way I agree it'd be better for the linter to enforce a single way

lib/internal/test_runner/utils.js

ljharb · 2022-12-16T05:46:19Z

lib/test/reporter/spec.js

+constinspectOptions={colors: true,breakLength: Infinity};
+
+constcolors={
+'__proto__': null,


Suggested change
'__proto__': null,
__proto__: null,
this should work the same since it's not computed, but there's no need to quote it

Using a string literal is not the same thing as using computed prop name (LiteralPropertyName : StringLiteral vs ComputedPropertyName : [ AssignmentExpression ]). I disagree with the above suggestion, I think it makes sense to keep all the keys quoted for consistency within the object. Anyway, no strong feelings so do however you like most.

There's an eslint rule for the object key quoting style, whatever style the project selects should be enforced. Certainly "consistent, but unquote if possible" is a lintable style, but the ecosystem practice is pretty overwhelmingly "only quote when necessary, consistency be damned" :-)

ljharb · 2022-12-16T05:46:27Z

lib/test/reporter/spec.js

+'test:diagnostic': blue,
+};
+constsymbols={
+'__proto__': null,


Suggested change
'__proto__': null,
__proto__: null,

lib/internal/test_runner/utils.js

doc/api/test.md

lib/test/reporter/dot.js

lib/test/reporter/spec.js

test/fixtures/test-runner/reporters.js

lib/internal/modules/utils.js

PR-URL: nodejs#45712Fixes: nodejs#45648 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

Backport-PR-URL: #46361 PR-URL: #45712Fixes: #45648 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

Notable changes: esm: * leverage loaders when resolving subsequent loaders (Maël Nison) #43772 fs: * (SEMVER-MINOR) add statfs() functions (Colin Ihrig) #46358 test_runner: * add reporters (Moshe Atlow) #45712 PR-URL: TBD

Notable changes: esm: * leverage loaders when resolving subsequent loaders (Maël Nison) #43772 fs: * (SEMVER-MINOR) add statfs() functions (Colin Ihrig) #46358 test_runner: * add reporters (Moshe Atlow) #45712 PR-URL: #46455

Notable changes: buffer: * (SEMVER-MINOR) add isAscii method (Yagiz Nizipli) #46046 deps: * upgrade npm to 9.4.0 (npm team) #46353 esm: * leverage loaders when resolving subsequent loaders (Maël Nison) #43772 fs: * (SEMVER-MINOR) add statfs() functions (Colin Ihrig) #46358 src,lib: * (SEMVER-MINOR) add constrainedMemory API for process (theanarkh) #46218 test_runner: * (SEMVER-MINOR) add reporters (Moshe Atlow) #45712 v8: * (SEMVER-MINOR) support gc profile (theanarkh) #46255 vm: * (SEMVER-MINOR) expose cachedDataRejected for vm.compileFunction (Anna Henningsen) #46320 PR-URL: #46455

PR-URL: nodejs/node#45712Fixes: nodejs/node#45648 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> (cherry picked from commit a1b27b25bb01aadd3fd2714e4b136db11b7eb85a)

PR-URL: nodejs#45712Fixes: nodejs#45648 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

PR-URL: #45712 Backport-PR-URL: #46839Fixes: #45648 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

Notable Changes: buffer: * (SEMVER-MINOR) add isAscii method (Yagiz Nizipli) #46046 doc,lib,src,test: * rename --test-coverage (Colin Ihrig) #46017 fs: * (SEMVER-MINOR) add statfs() functions (Colin Ihrig) #46358 src,lib: * (SEMVER-MINOR) add constrainedMemory API for process (theanarkh) #46218 test_runner: * add initial code coverage support (Colin Ihrig) #46017 * (SEMVER-MINOR) add reporters (Moshe Atlow) #45712 v8: * (SEMVER-MINOR) support gc profile (theanarkh) #46255 vm: * (SEMVER-MINOR) expose cachedDataRejected for vm.compileFunction (Anna Henningsen) #46320 PR-URL: #46920

PR-URL: #45712 Backport-PR-URL: #46839Fixes: #45648 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

Notable changes: buffer: * (SEMVER-MINOR) add isAscii method (Yagiz Nizipli) #46046 doc,lib,src,test: * rename --test-coverage (Colin Ihrig) #46017 fs: * (SEMVER-MINOR) add statfs() functions (Colin Ihrig) #46358 src,lib: * (SEMVER-MINOR) add constrainedMemory API for process (theanarkh) #46218 test_runner: * add initial code coverage support (Colin Ihrig) #46017 * (SEMVER-MINOR) add reporters (Moshe Atlow) #45712 v8: * (SEMVER-MINOR) support gc profile (theanarkh) #46255 vm: * (SEMVER-MINOR) expose cachedDataRejected for vm.compileFunction (Anna Henningsen) #46320 PR-URL: https://github.com/nodejs/node/pull/46920/commits

Notable changes: buffer: * (SEMVER-MINOR) add isAscii method (Yagiz Nizipli) #46046 doc,lib,src,test: * rename --test-coverage (Colin Ihrig) #46017 fs: * (SEMVER-MINOR) add statfs() functions (Colin Ihrig) #46358 src,lib: * (SEMVER-MINOR) add constrainedMemory API for process (theanarkh) #46218 test_runner: * add initial code coverage support (Colin Ihrig) #46017 * (SEMVER-MINOR) add reporters (Moshe Atlow) #45712 v8: * (SEMVER-MINOR) support gc profile (theanarkh) #46255 vm: * (SEMVER-MINOR) expose cachedDataRejected for vm.compileFunction (Anna Henningsen) #46320 PR-URL: #46920

nodejs/node#45712

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 Dec 2, 2022

MoLow added the test_runner Issues and PRs related to the test runner subsystem. label Dec 2, 2022

MoLow force-pushed the test_runner_reporters branch 2 times, most recently from 98598f0 to d79755cCompare December 2, 2022 12:50

MoLow commented Dec 2, 2022
View reviewed changes

lib/internal/test_runner/test.js Outdated Show resolvedHide resolved

cjihrig reviewed Dec 2, 2022
View reviewed changes

cjihrig reviewed Dec 4, 2022
View reviewed changes

MoLow force-pushed the test_runner_reporters branch from 3e3218f to 3ac1e2bCompare December 9, 2022 12:58

cjihrig reviewed Dec 9, 2022
View reviewed changes

MoLow force-pushed the test_runner_reporters branch 7 times, most recently from b65afb3 to c2412dfCompare December 11, 2022 06:59

MoLow mentioned this pull request Dec 11, 2022
test_runner: parse yaml #45815
Merged

MoLow added the dont-land-on-v14.x label Dec 11, 2022

MoLow force-pushed the test_runner_reporters branch 2 times, most recently from e9f0ccd to eb3702dCompare December 16, 2022 00:35

ljharb reviewed Dec 16, 2022
View reviewed changes

aduh95 reviewed Dec 16, 2022
View reviewed changes

lib/internal/test_runner/utils.js Outdated Show resolvedHide resolved
lib/internal/test_runner/utils.js Outdated Show resolvedHide resolved

aduh95 reviewed Dec 16, 2022
View reviewed changes

MoLow added backport-open-v18.x and removed backport-requested-v18.x labels Jan 26, 2023

ruyadorno mentioned this pull request Feb 1, 2023
v19.6.0 proposal #46455
Merged

MoLow added backported-to-v19.x and removed backport-open-v19.x labels Feb 1, 2023

ruyadorno added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 1, 2023

remcohaszing mentioned this pull request Feb 3, 2023
Support defining test reporter using environment variable #46484
Closed

juanarbol mentioned this pull request Mar 3, 2023
V18.15.0 proposal #46920
Merged

codebytere added a commit to electron/electron that referenced this pull request Apr 18, 2023
test_runner: add reporters
9372c22
nodejs/node#45712

		yieldreportTest(data.nesting,data.testNumber,'not ok',data.name,data.directive);
		yieldreportDetails(data.nesting,data.details);

	returnesmLoader.import(file,undefined,ObjectCreate(null));
	returnesmLoader.import(file,undefined,{__proto__: null});

Uh oh!

test_runner: add reporters#45712

test_runner: add reporters #45712

Uh oh!

Conversation

MoLow commented Dec 2, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MoLow commented Dec 3, 2022

Uh oh!

cjihrig commented Dec 3, 2022

Uh oh!

MoLow commented Dec 3, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cjihrig commented Dec 7, 2022

Uh oh!

MoLow commented Dec 9, 2022

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MoLow commented Dec 16, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ljharbDec 20, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MoLow commented Dec 2, 2022•
edited
Loading

ljharbDec 20, 2022•
edited
Loading