test_runner: changing spec from class to function

[shubham9411]

Fixes: #48112

The reporters module currently exports three items: tap, dot, and spec. However, while tap and dot are functional exports, spec is implemented as a class, requiring the use of the new keyword. This discrepancy can introduce inconsistency and make the usage less streamlined.

This PR aims to update the spec export in the reporters module to a functional implementation, similar to tap and dot.

[nodejs-github-bot]

Review requested:

• @nodejs/test_runner

[shubham9411]

I see there is another PR #48202 which fixes this by changing the reporters.js file where the spec api gets exposed. Let me know if this PR needs to be closed.

[atlowChemi]

@MoLow @cjihrig Do we still want/need this?

node/lib/internal/test_runner/utils.js

Lines 142 to 144 in c462147

	if (reporter?.prototype && ObjectGetOwnPropertyDescriptor(reporter.prototype, 'constructor')) {
	reporter = new reporter();
	}

The docs do not show passing a constructor as an option...

[MoLow]

yes, for custom reporters I guess

[atlowChemi]

Then I guess we might want to add an example for that to the docs?

[atlowChemi]

Doesn't this mean that calling run twice with spec now shares the failures between runs? (whereas previously you could instantiate a new reporter as per need)

[atlowChemi]

I think I would expect const spec to be a function creating these vars and return the new Transform. Does that make sense?

[shubham9411]

makes sense 👍🏻 will change

[shubham9411]

Hey @atlowChemi, I have updated the PR with the lates changes. Instead of using the new Transform, I have modified it to an async generator, similar to dot and tap reporters.

[aduh95]

If we want new specReporter to keep working (and I think we do, otherwise it's a breaking change), we cannot use a generator here, it has to be a function specReporter(source), async functions and (async) generators are not "constructable".

[shubham9411]

Just thinking out load, Is it possible to make specReporter function work consistently whether we use new specReporter() or just specReporter? I tried by it but it only works one way, can you suggest anything?

[aduh95]

That's what I meant, you need to use function specReporter(source) { /* … */ }, AFAIK it's the only thing is JS that is both callable and constructible.

[shubham9411]

ok sorry, I got confused, so I did made it like function specReporter(source) { /* … */ } and it works with new spec() and also work as spec(), but does not work standalone like the dot and tap. viz:

run(...).compose(new spec()); // this works
run(...).compose(spec()); // this works
run(...).compose(spec); // this does not work
run(...).compose(tap); // <- tap and dot work like this

the third one above is used like the dot and tap. If spec() is also acceptable, let me know I'll push the change. Or let me know if my understanding is correct or not.

[aduh95]

Can you share what's the error you're getting? That would help understand what might be the problem.

[cjihrig]

Is this still being worked on, or can this be closed?

[cjihrig]

No follow up. Closing. Feel free to reopen if you want to pick this back up though.