Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

report: fix formatting and add coverage for stdout/stderr #26522

Merged
merged 2 commits into from
Mar 10, 2019
Merged

report: fix formatting and add coverage for stdout/stderr #26522

merged 2 commits into from
Mar 10, 2019

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Mar 8, 2019

When process.report.filename is 'stdout' or 'stderr', the output runs together. This PR fixes that, and also improves test coverage.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

@cjihrig build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2816/pipeline

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Mar 8, 2019
@addaleax addaleax added the report Issues and PRs related to process.report. label Mar 8, 2019
@BridgeAR
Copy link
Member

BridgeAR commented Mar 8, 2019 •
edited by cjihrig
Loading

CI https://ci.nodejs.org/job/node-test-pull-request/21349/

EDIT(cjihrig): CI was green.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 8, 2019
@cjihrig
report: fix stdout/stderr output formatting
24286ea 
This commit prevents the report and "report complete" message
from being printed on the same line.

PR-URL: nodejs#26522
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@cjihrig
test: cover stdout/stderr usage in triggerReport()
86aed6d 
This commit adds coverage for the cases where
process.report.filename is 'stdout' or 'stderr'.

PR-URL: nodejs#26522
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@cjihrig
Copy link
Contributor Author

cjihrig commented Mar 10, 2019

Landed in 3194744...86aed6d.

@cjihrig cjihrig closed this Mar 10, 2019
@cjihrig cjihrig deleted the report branch March 10, 2019 19:59
@cjihrig cjihrig merged commit 86aed6d into nodejs:master Mar 10, 2019
BridgeAR pushed a commit that referenced this pull request Mar 13, 2019
@cjihrig @BridgeAR
report: fix stdout/stderr output formatting
541b097 
This commit prevents the report and "report complete" message
from being printed on the same line.

PR-URL: #26522
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BridgeAR pushed a commit that referenced this pull request Mar 13, 2019
@cjihrig @BridgeAR
test: cover stdout/stderr usage in triggerReport()
d6fc9ca 
This commit adds coverage for the cases where
process.report.filename is 'stdout' or 'stderr'.

PR-URL: #26522
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BridgeAR pushed a commit that referenced this pull request Mar 13, 2019
@cjihrig @BridgeAR
report: fix stdout/stderr output formatting
2eb6a40 
This commit prevents the report and "report complete" message
from being printed on the same line.

PR-URL: #26522
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BridgeAR pushed a commit that referenced this pull request Mar 13, 2019
@cjihrig @BridgeAR
test: cover stdout/stderr usage in triggerReport()
b048a38 
This commit adds coverage for the cases where
process.report.filename is 'stdout' or 'stderr'.

PR-URL: #26522
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Mar 13, 2019
Merged
BridgeAR pushed a commit that referenced this pull request Mar 14, 2019
@cjihrig @BridgeAR
report: fix stdout/stderr output formatting
ac81fd2 
This commit prevents the report and "report complete" message
from being printed on the same line.

PR-URL: #26522
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BridgeAR pushed a commit that referenced this pull request Mar 14, 2019
@cjihrig @BridgeAR
test: cover stdout/stderr usage in triggerReport()
5a0ed0b 
This commit adds coverage for the cases where
process.report.filename is 'stdout' or 'stderr'.

PR-URL: #26522
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. report Issues and PRs related to process.report.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@cjihrig @nodejs-github-bot @BridgeAR @jasnell @addaleax @richardlau