test_runner: refactor spec reporter to generator function - #50177
test_runner: refactor spec reporter to generator function#50177himself65 wants to merge 3 commits into
Conversation
Sorry, something went wrong.
|
Review requested:
|
Sorry, something went wrong.
There was a problem hiding this comment.
I will add test cases later, just wanna see if old test suites all pass now
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
There was a problem hiding this comment.
these functions require closure variables
Sorry, something went wrong.
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.
Sorry, something went wrong.
|
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). |
Sorry, something went wrong.
|
FWIW I think fixing the documentation would be preferable |
Sorry, something went wrong.
I think also should throw error when compose a non-stream variable instead of just do nothing |
Sorry, something went wrong.
|
I read the whole code flow, I think some type of mismatch error is ignored in the internal module. Fixing that |
Sorry, something went wrong.
|
|
||
| #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)
Sorry, something went wrong.
|
Closing this as I think this is a document issue(we didn't mention this) or node/stream issue(stream.compose will just ignore if the first parameter is a non-expected value) For more, see: #50187 |
Sorry, something went wrong.
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