test_runner: add reporters by MoLow · Pull Request #45712 · nodejs/node · GitHub

TODO:

docs
tests
support run api?

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.

. 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

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.

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)

👍🏻 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.

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

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.

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

@nodejs/test_runner I think this is ready for review

Suggested change

      
                return esmLoader.import(file, undefined, ObjectCreate(null));
          
                return esmLoader.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

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" :-)

Suggested change

      
              '__proto__': null,
          
              __proto__: null,

PR-URL: nodejs#45712 Fixes: nodejs#45648 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

Backport-PR-URL: #46361 PR-URL: #45712 Fixes: #45648 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

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#45712 Fixes: nodejs/node#45648 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> (cherry picked from commit a1b27b25bb01aadd3fd2714e4b136db11b7eb85a)

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 d79755c Compare December 2, 2022 12:50

MoLow commented Dec 2, 2022

View reviewed changes

Comment thread lib/internal/test_runner/test.js Outdated

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 3ac1e2b Compare 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 c2412df Compare 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 eb3702d Compare December 16, 2022 00:35

ljharb reviewed Dec 16, 2022

View reviewed changes

aduh95 reviewed Dec 16, 2022

View reviewed changes

Comment thread lib/internal/test_runner/utils.js Outdated

Comment thread lib/internal/test_runner/utils.js Outdated

aduh95 reviewed Dec 16, 2022

View reviewed changes

cjihrig added backport-requested-v19.x and removed dont-land-on-v19.x labels Jan 24, 2023

This was referenced Jan 25, 2023

test_runner: make built in reporters internal #46092

Merged

test_runner: add initial code coverage support #46017

Merged

juanarbol added the backport-requested-v18.x label Jan 25, 2023

juanarbol mentioned this pull request Jan 25, 2023

test: fix flaky test-runner-exit-code.js #46138

Merged

MoLow mentioned this pull request Jan 26, 2023

[v18.x backport] test_runner various features #46360

Closed

MoLow mentioned this pull request Jan 26, 2023

[v19.x backport] test_runner various features #46361

Closed

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

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

test_runner: add reporters by MoLow · Pull Request #45712 · nodejs/node · GitHub

Uh oh!

Uh oh!

test_runner: add reporters - #45712

test_runner: add reporters#45712nodejs-github-bot merged 5 commits intonodejs:mainnodejs/node:mainfrom MoLow:test_runner_reportersMoLow/node:test_runner_reportersCopy head branch name to clipboard

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!

ljharb Dec 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!

test_runner: add reporters#45712
nodejs-github-bot merged 5 commits into
nodejs:mainfrom
MoLow:test_runner_reporters

MoLow commented Dec 2, 2022 •
edited
Loading

ljharb Dec 20, 2022 •
edited
Loading