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: delegate stderr and stdout formatting to reporter by HinataKah0 · Pull Request #48045 · nodejs/node

Conversation

@HinataKah0

@HinataKah0 HinataKah0 commented May 17, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Introduce new TestsStream events test:stderr and test:stdout
to delegate stderr and stdout (e.g. console.log()) formatting
to the reporter. And patch existing reporters to:

  • Spec: output the message as it is
  • TAP: stay the same with existing test:diagnostic

Fixes: #48011

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss, tonivj5, and vassudanagunta reacted with thumbs up emoji
All reactions
  • 👍 3 reactions
@nodejs-github-bot

nodejs-github-bot commented May 17, 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 lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels May 17, 2023
HinataKah0
HinataKah0 commented May 17, 2023
View reviewed changes
Comment thread lib/internal/test_runner/tests_stream.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
cjihrig
cjihrig reviewed May 17, 2023
View reviewed changes
Comment thread doc/api/test.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread doc/api/test.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
MoLow
MoLow reviewed May 17, 2023
View reviewed changes
Comment thread doc/api/test.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
MoLow
MoLow reviewed May 17, 2023
View reviewed changes
Comment thread lib/internal/test_runner/runner.js Outdated

@MoLow MoLow May 17, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

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.

as you have mentioned, nesting is redundant here.
also, I think we should stop breaking this into lines and delegate that to the TAP reporter (and leave as is in spec reporter) - but that dosnt have to happen in this PR

Sorry, something went wrong.

Uh oh!

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

@HinataKah0 HinataKah0 May 19, 2023

Copy link
Copy Markdown
Contributor 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.

I think we should stop breaking this into lines and delegate that to the TAP reporter (and leave as is in spec reporter)

Sorry I don't get the spec reporter part. By "leaving as it is" are you referring to printing \n as \\n?

Sorry, something went wrong.

Uh oh!

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

@MoLow MoLow May 19, 2023

Copy link
Copy Markdown
Member

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.

Sorry I don't get the spec reporter part. By "leaving as it is" are you referring to printing \n as \n?

I was referring to the output being split into lines before passing it to the reporter:
https://github.com/nodejs/node/blob/3473edc0bec636d7aa8a39c1df464feab0c2f72d/lib/internal/test_runner/runner.js#L283

but as written, it is also ok to not include it in this PR

Sorry, something went wrong.

Uh oh!

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

@HinataKah0 HinataKah0 May 19, 2023

Copy link
Copy Markdown
Contributor 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.

I'll address it in separate PR 👍

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
MoLow
MoLow reviewed May 17, 2023
View reviewed changes
Comment thread lib/internal/test_runner/runner.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
MoLow
MoLow reviewed May 17, 2023
View reviewed changes
Comment thread test/fixtures/test-runner/output/stdout.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@MoLow MoLow added the test_runner Issues and PRs related to the test runner subsystem. label May 18, 2023
@MoLow MoLow mentioned this pull request May 18, 2023
test_runner: align stdout and std error with and without --test #48057
Closed
@HinataKah0

HinataKah0 commented May 19, 2023

Copy link
Copy Markdown
Contributor Author

hopefully, with #48045 we can make node --test test.js behave the same as node test.js in terms of console logs

Hi @MoLow , so we want to make the output format of both with & without --test same, right? Maybe without the prefix # or the symbol?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@HinataKah0
HinataKah0 force-pushed the tests-stream-new-event branch from 6e503ca to 3473edc Compare May 19, 2023 02:30
MoLow
MoLow reviewed May 19, 2023
View reviewed changes
Comment thread doc/api/test.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
MoLow
MoLow reviewed May 19, 2023
View reviewed changes
Comment thread doc/api/test.md Resolved

Uh oh!

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

MoLow commented May 19, 2023

Copy link
Copy Markdown
Member

so we want to make the output format of both with & without --test same, right? Maybe without the prefix # or the symbol?

I think TAP should stay the same as today, and spec should just pipe the original data. that is the goal
HinataKah0 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@HinataKah0
HinataKah0 force-pushed the tests-stream-new-event branch from 3473edc to 16f1020 Compare May 19, 2023 14:58
@HinataKah0

HinataKah0 commented May 19, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

For the Spec reporter, it looks like this:
Screenshot 2023-05-19 at 10 57 10 PM

I don't think we have any test for console.log Spec reporter because after I changed to output the original data as it is, it doesn't break any test.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@HinataKah0 HinataKah0 changed the title test_runner: delegate stdout formatting to reporter test_runner: delegate stderr and stdout formatting to reporter May 19, 2023
@HinataKah0
test_runner: delegate stderr and stdout formatting to reporter
67f3543 
Introduce new `TestsStream` events `test:stderr` and `test:stdout`
to delegate `stderr` and `stdout` (e.g. `console.log()`) formatting
to the reporter. And patch existing reporters to:
- Spec: output the message as it is
- TAP: stay the same with existing `test:diagnostic`
@HinataKah0
HinataKah0 force-pushed the tests-stream-new-event branch from 16f1020 to 67f3543 Compare May 21, 2023 09:02
MoLow
MoLow approved these changes May 21, 2023
View reviewed changes
cjihrig
cjihrig reviewed May 21, 2023
View reviewed changes
Comment thread doc/api/test.md
### Event: `'test:stderr'`

* `data` {Object}
* `file` {string} The path of the test file.

@cjihrig cjihrig May 21, 2023

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.

For the test:diagnostic event, this can also be undefined. Is that possible for these new events?

Sorry, something went wrong.

Uh oh!

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

@MoLow MoLow May 21, 2023

Copy link
Copy Markdown
Member

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.

no. test:diagnostic can be produced inside the REPL. these new events are only useful with --test

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
HinataKah0 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@MoLow
MoLow requested a review from cjihrig May 22, 2023 15:51
cjihrig
cjihrig approved these changes May 22, 2023
View reviewed changes
@MoLow MoLow added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels May 22, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label May 22, 2023
@nodejs-github-bot

nodejs-github-bot commented May 22, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/51916/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot mentioned this pull request May 23, 2023
CI Reliability 2023-05-23 nodejs/reliability#574
Open
22 tasks
@nodejs-github-bot

nodejs-github-bot commented May 23, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/51920/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MoLow MoLow added the commit-queue PRs queued for automated landing through the Commit Queue. label May 23, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label May 23, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 1229a22 into nodejs:main May 23, 2023
@nodejs-github-bot

nodejs-github-bot commented May 23, 2023

Copy link
Copy Markdown
Collaborator

Landed in 1229a22
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@HinataKah0
HinataKah0 deleted the tests-stream-new-event branch May 23, 2023 13:37
@HinataKah0 HinataKah0 mentioned this pull request May 23, 2023
test_runner: delegate split message to reporter #48140
Closed
This was referenced May 24, 2023
CI Reliability 2023-05-24 nodejs/reliability#575
Open
CI Reliability 2023-05-25 nodejs/reliability#576
Open
CI Reliability 2023-05-26 nodejs/reliability#577
Open
CI Reliability 2023-05-27 nodejs/reliability#578
Open
CI Reliability 2023-05-28 nodejs/reliability#579
Open
CI Reliability 2023-05-29 nodejs/reliability#580
Open
CI Reliability 2023-05-30 nodejs/reliability#581
Open
targos pushed a commit that referenced this pull request May 30, 2023
@HinataKah0 @targos
test_runner: delegate stderr and stdout formatting to reporter
2b797a6 
Introduce new `TestsStream` events `test:stderr` and `test:stdout`
to delegate `stderr` and `stdout` (e.g. `console.log()`) formatting
to the reporter. And patch existing reporters to:
- Spec: output the message as it is
- TAP: stay the same with existing `test:diagnostic`

PR-URL: #48045
Fixes: #48011
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@github-actions github-actions Bot mentioned this pull request May 31, 2023
CI Reliability 2023-05-31 nodejs/reliability#582
Open
22 tasks
@targos targos mentioned this pull request Jun 4, 2023
v20.3.0 release proposal #48332
Merged
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
@HinataKah0 @danielleadams
test_runner: delegate stderr and stdout formatting to reporter
6bd758f 
Introduce new `TestsStream` events `test:stderr` and `test:stdout`
to delegate `stderr` and `stdout` (e.g. `console.log()`) formatting
to the reporter. And patch existing reporters to:
- Spec: output the message as it is
- TAP: stay the same with existing `test:diagnostic`

PR-URL: #48045
Fixes: #48011
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
@HinataKah0 @MoLow
test_runner: delegate stderr and stdout formatting to reporter
b18a78c 
Introduce new `TestsStream` events `test:stderr` and `test:stdout`
to delegate `stderr` and `stdout` (e.g. `console.log()`) formatting
to the reporter. And patch existing reporters to:
- Spec: output the message as it is
- TAP: stay the same with existing `test:diagnostic`

PR-URL: nodejs#48045
Fixes: nodejs#48011
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@danielleadams danielleadams mentioned this pull request Jul 10, 2023
v18.17.0 release proposal #48694
Merged
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@HinataKah0 @Ceres6
test_runner: delegate stderr and stdout formatting to reporter
bc17cee 
Introduce new `TestsStream` events `test:stderr` and `test:stdout`
to delegate `stderr` and `stdout` (e.g. `console.log()`) formatting
to the reporter. And patch existing reporters to:
- Spec: output the message as it is
- TAP: stay the same with existing `test:diagnostic`

PR-URL: nodejs#48045
Fixes: nodejs#48011
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@HinataKah0 @Ceres6
test_runner: delegate stderr and stdout formatting to reporter
92bfbec 
Introduce new `TestsStream` events `test:stderr` and `test:stdout`
to delegate `stderr` and `stdout` (e.g. `console.log()`) formatting
to the reporter. And patch existing reporters to:
- Spec: output the message as it is
- TAP: stay the same with existing `test:diagnostic`

PR-URL: nodejs#48045
Fixes: nodejs#48011
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MoLow MoLow MoLow approved these changes
@aduh95 aduh95 aduh95 left review comments
@cjihrig cjihrig cjihrig approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Test Runner: delegate arbitrary output formatting to each reporter

Uh oh!

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

5 participants

@HinataKah0 @nodejs-github-bot @MoLow @cjihrig @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.