test_runner: refactor spec reporter to generator function #50177
Conversation
|
Review requested:
|
nodejs-github-bot
added
needs-ci
himself65
force-pushed
the
himself65/spec
branch
2 times, most recently
from
0520c59 to
c67f4fb
Compare
himself65
force-pushed
the
himself65/spec
branch
from
c67f4fb to
dba68c0
Compare
himself65
changed the title
| let failedTests = []; | ||
| let cwd = process.cwd(); | ||
|
|
||
| function indent(nesting) { |
There was a problem hiding this comment.
this means all of these functions are defined anew every time this reporter is called, instead of just once at module startup.
There was a problem hiding this comment.
these functions require closure variables
There was a problem hiding this comment.
then if you're switching to a non-class-based model, the functions should take everything as arguments, so they can be defined at module level.
|
Are reporters stable? If so, this is semver-major and we should document the current behavior. Note That I think it's possible to make it a non-semver major change by using a function and old school inheritance (like Transform itself). |
|
FWIW I think fixing the documentation would be preferable |
I think also should throw error when compose a non-stream variable instead of just do nothing |
|
I read the whole code flow, I think some type of mismatch error is ignored in the internal module. Fixing that |
|
|
||
| #indent(nesting) { | ||
| let value = this.#indentMemo.get(nesting); | ||
| module.exports = async function *specReporter(source) { |
There was a problem hiding this comment.
This would still need to be a transform stream, API wise (can be from a generator)
|
Closing this as I think this is a document issue(we didn't mention this) or node/stream issue( For more, see: #50187 |
Fixes: #50176
Upstream: #48112 #48202
Even if the original code is 100% correct, the spec reporter was a class but the other nodejs internal reporters are all async functions. This will be a little misleading like
stream.compose(spec)is incorrect because spec is a class, not a generator.I think all the internal implementation should be in the same way to make the user side more clear and easy
/cc @mcollina