Repository navigation
Specify that spec reporter is a class and needs to be instantiate for usage with run #48112
Description
Activity
are you interested in opening a PR for this?
Not sure how to get started with PR @MoLow
But I think we also need to discuss if api (and behavior) of all
test/reportersexports is correct. Ideally all exported reporters should work in same manner? There should be no need to donew spec().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?
node:test/reportershas three exports currently,spec,tapanddot(may be more in the future). Two of them can be used directly, butspecneeds be instantiate. If you ask me this is an odd api, ideallyspecshould 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 :)
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).
Reacted by Moshe Atlow and TethetHey @MoLow, I see you added good first issue tag to this issue. Can I take this?
Yeah 👍
I am thinking of replacing
SpecReporterclass with a function, as @aduh95 suggested.- added a commit that references this issue
on May 27, 2023 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) :
- If we make change to reporter/spec.js : (@line-no:143) test_runner/utils.js
- 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 ?
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.
Reacted by Sumeet Kaul and Shailendra2 remaining items
- 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.
Reacted by Sumeet KaulYou 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 ..git commit --amendallows to edit a commits message and contentReacted by Sumeet KaulOr you can use
git fetch https://github.com/nodejs/node.git && git rebase FETCH_HEAD -i, and there you can replacepickwithrewordon 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.Reacted by Moshe Atlow, Sumeet Kaul and One-Seeis this issue still open?
- added a commit that references this issue
on Aug 17, 2023 - added a commit that references this issue
on Sep 10, 2023 - added a commit that references this issue
on Nov 27, 2023 - added 2 commits that reference this issue
on Apr 25, 2024
Affected URL(s)
https://nodejs.org/api/test.html#test-reporters
Description of the problem
When importing
spectest reporter, you need to instantiate it otherwise you will get no output, for exampleimport { 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
specreporter is exported as a class unliketapanddotreporters.