GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — test_runner: handled change for exposing 'spec' reporter by Sumi0 · Pull Request #48202 · nodejs/node

Conversation

@Sumi0

@Sumi0 Sumi0 commented May 27, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
sushantdhiman and bricss reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
Sumeet Kaul
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

ghost commented May 27, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/test_runner
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to Node.js core tests and test infrastructure. labels May 27, 2023
aduh95
aduh95 reviewed May 27, 2023
View reviewed changes
Comment thread lib/test/reporters.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Sumeet Kaul
fixup! test_runner: handled change for exposing 'spec' reporter
91518ee
@shubham9411 shubham9411 mentioned this pull request May 27, 2023
test_runner: changing spec from class to function #48208
Closed
@MoLow MoLow mentioned this pull request May 28, 2023
Specify that spec reporter is a class and needs to be instantiate for usage with run #48112
Closed
aduh95
aduh95 reviewed May 28, 2023
View reviewed changes
Comment thread lib/test/reporters.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Sumeet Kaul
fixup2! test_runner: handled change for exposing 'spec' reporter
e257f77
MoLow
MoLow approved these changes Jun 1, 2023
View reviewed changes
@MoLow
MoLow requested a review from aduh95 June 1, 2023 06:47
@Sumi0

ghost commented Jun 1, 2023

Copy link
Copy Markdown
Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MoLow

ghost commented Jun 1, 2023

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Sumi0

ghost commented Jun 1, 2023

Copy link
Copy Markdown
Author

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)
      ...
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
aduh95
aduh95 reviewed Jun 1, 2023
View reviewed changes
Comment thread lib/test/reporters.js
Comment on lines +24 to +25
value: spec ?? function SpecReporter() {
return ReflectConstruct(require('internal/test_runner/reporter/spec'), arguments);

ghost Jun 1, 2023 •
edited by aduh95
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
MoLow reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

ghost Jun 1, 2023

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

ghost Jul 4, 2023

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@atlowChemi atlowChemi mentioned this pull request Jun 30, 2023
Programmatically using the spec reporter from node:test/reporters doesn't output anything. #48585
Closed
@cjihrig

ghost commented Aug 15, 2023

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@himself65 himself65 mentioned this pull request Oct 13, 2023
test_runner: refactor spec reporter to generator function #50177
Closed
@aduh95 aduh95 added the stalled Issues and PRs manually marked as stalled and scheduled for automatic closure. label May 11, 2024
@github-actions

ghost commented May 11, 2024

Copy link
Copy Markdown
Contributor

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@benjamingr

ghost commented May 11, 2024

Copy link
Copy Markdown
Member

This was already superseded by #49184
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

needs-ci PRs that need a full CI run. stalled Issues and PRs manually marked as stalled and scheduled for automatic closure. test Issues and PRs related to Node.js core tests and test infrastructure.

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

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@Sumi0 @nodejs-github-bot @MoLow @cjihrig @benjamingr @aduh95
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.