test_runner: changing spec from class to function by shubham9411 · Pull Request #48208 · nodejs/node

shubham9411 · 2023-05-27T22:17:15Z

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 · 2023-05-27T22:17:19Z

Review requested:

@nodejs/test_runner

…11/node into making-specReporter-functional

shubham9411 · 2023-05-27T22:35:41Z

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 · 2023-05-28T09:13:04Z

lib/internal/test_runner/reporter/spec.js

  'arrow:right': '\u25B6 ',
  'hyphen:minus': '\uFE63 ',
 };
-class SpecReporter extends Transform {


@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...

yes, for custom reporters I guess

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

atlowChemi · 2023-05-28T09:21:05Z

lib/internal/test_runner/reporter/spec.js

+const stack = [];
+const reported = [];
+const indentMemo = new SafeMap();
+const failedTests = [];


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)

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

makes sense 👍🏻 will change

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.

…porter-functional

aduh95 · 2023-07-15T13:39:28Z

lib/internal/test_runner/reporter/spec.js

-  constructor() {
-    super({ writableObjectMode: true });
-  }
+async function* specReporter(source) {


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".

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?

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.

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.

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

cjihrig · 2023-08-11T15:43:50Z

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

cjihrig · 2023-08-15T13:45:47Z

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

Fixes: nodejs#48112 Ref: nodejs#48208

Fixes: #48112 Ref: #48208 PR-URL: #49184 Refs: #48208 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

Fixes: nodejs/node#48112 Ref: nodejs/node#48208 PR-URL: nodejs/node#49184 Refs: nodejs/node#48208 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

test_runner: changing spec from class to function

3ca1251

nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels May 27, 2023

shubham9411 added 2 commits May 28, 2023 03:51

test_runner: changing spec from class to function

cd87a5b

Merge branch 'making-specReporter-functional' of github.com:shubham94…

f04482d

…11/node into making-specReporter-functional

aduh95 added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 28, 2023

atlowChemi reviewed May 28, 2023

View reviewed changes

atlowChemi mentioned this pull request Jun 30, 2023

Programmatically using the spec reporter from node:test/reporters doesn't output anything. #48585

Closed

shubham9411 added 5 commits July 10, 2023 00:06

Merge branch 'main' of github.com:shubham9411/node into making-specRe…

b4a450b

…porter-functional

spec transform

4b33ac5

Merge branch 'main' of github.com:shubham9411/node into making-specRe…

5e43fee

…porter-functional

Fixing tests

d6c0e92

fix: eslint issues

e1d57a9

aduh95 reviewed Jul 15, 2023

View reviewed changes

cjihrig closed this Aug 15, 2023

atlowChemi added a commit to atlowChemi/node that referenced this pull request Aug 15, 2023

test_runner: expose test_runner as newable function

2c9b2fc

Fixes: nodejs#48112 Ref: nodejs#48208

atlowChemi mentioned this pull request Aug 15, 2023

test_runner: expose spec reporter as newable function #49184

Merged

atlowChemi added a commit to atlowChemi/node that referenced this pull request Aug 15, 2023

test_runner: expose test_runner as newable function

7a2f61e

Fixes: nodejs#48112 Ref: nodejs#48208

atlowChemi added a commit to atlowChemi/node that referenced this pull request Aug 15, 2023

test_runner: expose test_runner as newable function

f217312

Fixes: nodejs#48112 Ref: nodejs#48208

atlowChemi added a commit to atlowChemi/node that referenced this pull request Aug 16, 2023

test_runner: expose spec reporter as newable function

193c63a

Fixes: nodejs#48112 Ref: nodejs#48208

UlisesGascon pushed a commit that referenced this pull request Sep 10, 2023

test_runner: expose spec reporter as newable function

283f280

Fixes: #48112 Ref: #48208 PR-URL: #49184 Refs: #48208 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

targos pushed a commit that referenced this pull request Nov 27, 2023

test_runner: expose spec reporter as newable function

95cc98e

Fixes: #48112 Ref: #48208 PR-URL: #49184 Refs: #48208 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

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

Uh oh!

Conversation

shubham9411 commented May 27, 2023

Uh oh!

nodejs-github-bot commented May 27, 2023

Uh oh!

shubham9411 commented May 27, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Aug 11, 2023

Uh oh!

cjihrig commented Aug 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants