Specify that spec reporter is a class and needs to be instantiate for usage with run

[sushantdhiman]

Affected URL(s)

https://nodejs.org/api/test.html#test-reporters

Description of the problem

When importing spec test reporter, you need to instantiate it otherwise you will get no output, for example

import { run } from "node:test";
import { tap, spec } from "node:test/reporters";
import { resolve } from "path";

const files = [resolve("./test/test.js")];

run({
  files,
  concurrency: 1,
  timeout: 10000,
})
 --- .compose(spec)
+++.compose(new spec())
  .pipe(process.stdout);

Documentation should indicate that spec reporter is exported as a class unlike tap and dot reporters.

[MoLow]

are you interested in opening a PR for this?

[sushantdhiman]

Not sure how to get started with PR @MoLow

But I think we also need to discuss if api (and behavior) of all test/reporters exports is correct. Ideally all exported reporters should work in same manner? There should be no need to do new spec().

[MoLow]

But I think we also need to discuss if api (and behavior) of all test/reporters exports is correct. Ideally all exported reporters should work in same manner? There should be no need to do new spec().

why?

[sushantdhiman]

node:test/reporters has three exports currently, spec, tap and dot (may be more in the future). Two of them can be used directly, but spec needs be instantiate. If you ask me this is an odd api, ideally spec should work directly.

But it is just a matter of preference. I have reported oddity with the API docs, it is up to the maintainers how they wish to fix this :)

[aduh95]

All lowercase name usually means a function, classes would typically use PascalCase. I agree this is odd, we should fix it either by renaming it and deprecating the old name, or replace it with a function. The latter has the advantage of being doable without any breaking change (hopefully).

[shubham9411]

Hey @MoLow, I see you added good first issue tag to this issue. Can I take this?

[MoLow]

Yeah 👍

[shubham9411]

I am thinking of replacing SpecReporter class with a function, as @aduh95 suggested.

[Sumi0]

Hi @MoLow, I was curious to take up this issue.
I checked the codebase thoroughly to see any breaking changes.

They were in two files, according to where we want to make change (signature/ exposure) :

1. If we make change to reporter/spec.js : (@line-no:143) test_runner/utils.js
2. If we make change to reporters.js : (@line-no:79) test-runner-run.mjs

So instead of making amends to the original signature of 'spec' reporter, I have made change where it is getting exposed in reporters.js.

Kindly check whether this approach is feasible and correct ?

[shubham9411]

Hi @Sumi0, I didn't see your PR that was opened. Please let me know if I need to close my PR, which changes the spec class to a function.

[Sumi0]

That's upon @MoLow and @aduh95 .
My PoV is to not change 'spec' reporter's signature to a function.

[MoLow]

I personally prefer #48202

[Sumi0]

I had 2 questions @MoLow, @aduh95 :

1. How do maintainers come to know that, there is an PR awaiting their approval for CI workflow?
2. How do you follow this linting guideline --Wrap all other lines at 72 columns (except for long URLs)-- before making a commit from Github GUI ? What's the process to assert this guideline ?

[aduh95]

1. How do maintainers come to know that, there is an PR awaiting their approval for CI workflow?

There would be a GitHub notification if they are subscribed to that PR (and they would typically be because the bot pings the team "owning" the modified files).

2. How do you follow this linting guideline --Wrap all other lines at 72 columns (except for long URLs)-- before making a commit from Github GUI ? What's the process to assert this guideline ?

You count the chars I guess, and when the line grows larger than 72 chars, you add a line return at the start of the word. If that guideline is not met, the CQ will refuse to land the PR.

[Sumi0]

You count the chars I guess, and when the line grows larger than 72 chars, you add a line return at the start of the word. If that guideline is not met, the CQ will refuse to land the PR.

So how should one change the commit message afterwards ?
Since I know, my CI test for my first commit linting failed ..

[MoLow]

git commit --amend allows to edit a commits message and content

[aduh95]

Or you can use git fetch https://github.com/nodejs/node.git && git rebase FETCH_HEAD -i, and there you can replace pick with reword on the first commit. It's also OK if you don't want to fix it yourself, it can be fixed by someone else, but it's of course more convenient for everyone if you are able to do it yourself.

[AryanG210]

is this issue still open?