-
Notifications
You must be signed in to change notification settings - Fork 28.9k
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
test_runner: add reporters #45712
test_runner: add reporters #45712
Conversation
98598f0
to
d79755c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
can you elaborate? what alternative approach did you have in mind?
👍🏻 I think the main challenge is a DX challenge, not a technical challenge - so lets continue the brainstorm in #45648 before merging this |
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. |
that is pretty much what I have suggested here, maybe I have not described it well |
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. |
3e3218f
to
3ac1e2b
Compare
@cjihrig pushed another iteration implementing the discussed here, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/tap.js
Outdated
yield reportTest(data.nesting, data.testNumber, 'not ok', data.name, data.directive); | ||
yield reportDetails(data.nesting, data.details); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be yielding twice here? Same question for 'test:pass'
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just kept the same behavior as before, we sent this as two different chunks
b65afb3
to
c2412df
Compare
e9f0ccd
to
eb3702d
Compare
@nodejs/test_runner I think this is ready for review |
const { pathToFileURL } = require('internal/url'); | ||
const { isAbsolute } = require('path'); | ||
const file = isAbsolute(filePath) ? pathToFileURL(filePath).href : filePath; | ||
return esmLoader.import(file, undefined, ObjectCreate(null)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return esmLoader.import(file, undefined, ObjectCreate(null)); | |
return esmLoader.import(file, undefined, { __proto__: null }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish we could have just 1 way to do this—I don't care which of the 3–4 I'm aware of.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
const inspectOptions = { colors: true, breakLength: Infinity }; | ||
|
||
const colors = { | ||
'__proto__': null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'__proto__': null, | |
__proto__: null, |
this should work the same since it's not computed, but there's no need to quote it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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" :-)
'test:diagnostic': blue, | ||
}; | ||
const symbols = { | ||
'__proto__': null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'__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>
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
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)
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)
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)
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)
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)
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>
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
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
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
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
Fixes: #45648
TODO:
run
api?