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

src: correct json writer the place on process.report #28433

Closed
wants to merge 3 commits into from
Closed

src: correct json writer the place on process.report #28433

wants to merge 3 commits into from

Conversation

himself65
Copy link
Member

@himself65 himself65 commented Jun 26, 2019 •
edited
Loading

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

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. report Issues and PRs related to process.report. labels Jun 26, 2019
addaleax
addaleax approved these changes Jun 26, 2019
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Do you have a test case that we could maybe add?

@gireeshpunathil
Copy link
Member

Do you have a test case that we could maybe add?

IIUC, there was nothing broken (as there isn't anything after the while loop, in the else block) , this change aligns the code better.

@addaleax
Copy link
Member

@gireeshpunathil If we take the if path, we’d previously have had an json_arrayend without json_arraystart, which seems like a bug to me

@gireeshpunathil
Copy link
Member

@addaleax - ah, I see! thnx!

@himself65
Copy link
Member Author

himself65 commented Jun 26, 2019 •
edited
Loading

I add tests to the new commits

on Node v12.4.0

const error = new Error();
error.stack = 'only one line';
process.report.writeReport(error);

and the output will like this

  },
  "javascriptStack": {
    "message": "only one line"
  }
]
},
"nativeStack": [

@richardlau
Copy link
Member

Test failure is because we end up with two json_objectend() calls (on lines 369 and 387). The one on line 369 can be removed.

@himself65 himself65 changed the title src: fix write json_arraryend at a wrong place src: correct json writer the place on process.report Jun 26, 2019
@himself65
Copy link
Member Author

author ready🤔

@himself65 himself65 mentioned this pull request Jun 26, 2019
Closed
3 tasks
@richardlau richardlau added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 26, 2019
@nodejs-github-bot
Copy link
Collaborator

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

Trott pushed a commit to Trott/io.js that referenced this pull request Jul 6, 2019
@himself65 @Trott
src: correct json writer placement in process.report
7fa9abf 
PR-URL: nodejs#28433
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Trott pushed a commit to Trott/io.js that referenced this pull request Jul 6, 2019
@himself65 @Trott
test: check writeReport when error with one line stack
1c989c9 
PR-URL: nodejs#28433
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Trott pushed a commit to Trott/io.js that referenced this pull request Jul 6, 2019
@himself65 @Trott
test: check getReport when error with one line stack
c25cccf 
PR-URL: nodejs#28433
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@Trott
Copy link
Member

Trott commented Jul 6, 2019

Landed in 6aafee1...c25cccf

@Trott Trott closed this Jul 6, 2019
@himself65 himself65 deleted the array branch July 6, 2019 08:55
targos pushed a commit that referenced this pull request Jul 20, 2019
@himself65 @targos
src: correct json writer placement in process.report
48c369b 
PR-URL: #28433
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this pull request Jul 20, 2019
@himself65 @targos
test: check writeReport when error with one line stack
793163e 
PR-URL: #28433
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this pull request Jul 20, 2019
@himself65 @targos
test: check getReport when error with one line stack
5980373 
PR-URL: #28433
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
This was referenced Jul 23, 2019
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott approved these changes

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

@richardlau richardlau richardlau approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@BridgeAR BridgeAR BridgeAR 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.
8 participants
@himself65 @gireeshpunathil @addaleax @richardlau @nodejs-github-bot @Trott @cjihrig @BridgeAR