Skip to content

test_runner: handled change for exposing 'spec' reporter#48202

Closed
Sumi0 wants to merge 3 commits intonodejs:mainfrom
Sumi0:main
Closed

test_runner: handled change for exposing 'spec' reporter#48202
Sumi0 wants to merge 3 commits intonodejs:mainfrom
Sumi0:main

Conversation

@Sumi0
Copy link

@Sumi0 Sumi0 commented May 27, 2023 •
edited
Loading

Fixes: Issue#48112

Other reporters (dot, tap) by signature are a function while 'spec'
reporter is a ES6 class.

This behaviour of api spec is causing difference in semantics while
consumption since it has not been addressed anywhere in the document
(it has to be instantiated).

Instead of making changes in the signature of spec.js, i have proposed
changes where the 'spec' reporter gets exposed in reporter.js

Refs: reporter/spec.js
Refs: (@line-no:143) test_runner/utils.js

@Sumi0
test_runner: handled change for exposing 'spec' reporter
4ebfc7b 
Other reporters (dot, tap) by signature are a function while 'spec' reporter is a ES6 class.

This behaviour of api spec is causing difference in semantics while consumption since it has not been addressed anywhere in the document (it has to be instantiated).

Instead of making changes in the signature of spec.js, i have proposed changes where the 'spec' reporter gets exposed in reporter.js

Fixes: nodejs#48112
Refs: https://github.com/nodejs/node/blob/main/lib/internal/test_runner/reporter/spec.js
Refs: (@line-no:143) https://github.com/nodejs/node/blob/main/lib/internal/test_runner/utils.js
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 27, 2023

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels May 27, 2023
aduh95
aduh95 reviewed May 27, 2023
View reviewed changes
@shubham9411 shubham9411 mentioned this pull request May 27, 2023
Closed
@MoLow MoLow mentioned this pull request May 28, 2023
Closed
aduh95
aduh95 reviewed May 28, 2023
View reviewed changes
@MoLow MoLow requested a review from aduh95 June 1, 2023 06:47
@Sumi0
Copy link
Author

Sumi0 commented Jun 1, 2023

I do not understand why is the test failing @line-no:79 test-runner-run.mjs.
Where could be the root-cause ? @MoLow @aduh95

@MoLow
Copy link
Member

MoLow commented Jun 1, 2023

you should debug locally with ./node test/parallel/test-runner-run.mjs

@Sumi0
Copy link
Author

Sumi0 commented Jun 1, 2023

I do not understand why is the test failing @line-no:79 test-runner-run.mjs.
Where could be the root-cause ? @MoLow @aduh95

    not ok 9 - should be piped with spec
      ---
      duration_ms: 1767.493155
      failureType: 'cancelledByParent'
      error: 'Promise resolution is still pending but the event loop has already resolved'
      code: 'ERR_TEST_FAILURE'
      stack: |-
        process.emit (node:events:523:35)
      ...

aduh95
aduh95 reviewed Jun 1, 2023
View reviewed changes
lib/test/reporters.js
Comment on lines +24 to +25
value: spec ?? function SpecReporter() {
return ReflectConstruct(require('internal/test_runner/reporter/spec'), arguments);
Copy link
Contributor

@aduh95 aduh95 Jun 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
value: spec ?? function SpecReporter() {
return ReflectConstruct(require('internal/test_runner/reporter/spec'), arguments);
value: function SpecReporter() {
spec ??= require('internal/test_runner/reporter/spec');
return ReflectConstruct(spec, arguments, new.target);

(not sure about the new.target)

Copy link
Author

@Sumi0 Sumi0 Jun 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me run full test this time on my local and check.

Copy link
Author

@Sumi0 Sumi0 Jul 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@atlowChemi I have tried all versions of returning instance of a class (imported with require), either with new keyword or with Reflect.construct, and both are not working.
I am not able to identify the Root cause.

@atlowChemi atlowChemi mentioned this pull request Jun 30, 2023
Closed
@cjihrig
Copy link
Contributor

cjihrig commented Aug 15, 2023

@Sumi0 are you still planning to work on this, or should it be closed?

@himself65 himself65 mentioned this pull request Oct 13, 2023
Closed
@aduh95 aduh95 added the stalled Issues and PRs that are stalled. label May 11, 2024
@github-actions
Copy link
Contributor

github-actions bot commented May 11, 2024

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

@benjamingr
Copy link
Member

benjamingr commented May 11, 2024

This was already superseded by #49184

@benjamingr benjamingr closed this May 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aduh95 aduh95 aduh95 left review comments

@MoLow MoLow MoLow approved these changes

Assignees

No one assigned

Labels

needs-ci PRs that need a full CI run. stalled Issues and PRs that are stalled. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

6 participants

@Sumi0 @nodejs-github-bot @MoLow @cjihrig @benjamingr @aduh95