FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

assert: fix throws trace by BridgeAR · Pull Request #18595 · nodejs/node · GitHub

/ node Public

assert: fix throws trace - #18595

Closed
BridgeAR wants to merge 2 commits into
nodejs:masterfrom
BridgeAR:assert-throws-stack
ConversationCommits (2)ChecksFiles changed
Closed

assert: fix throws trace#18595
BridgeAR wants to merge 2 commits into
nodejs:masterfrom
BridgeAR:assert-throws-stack

Conversation

BridgeAR commented Feb 6, 2018

Copy link
Copy Markdown
Member

The current stack trace thrown in case assert.throws(fn, object)
is used did not filter the stack trace. This fixes it.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

assert

assert: fix throws trace
d2b6f4d 
The current stack trace thrown in case `assert.throws(fn, object)`
is used did not filter the stack trace. This fixes it.
nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label Feb 6, 2018

BridgeAR commented Feb 6, 2018

Copy link
Copy Markdown
Member Author

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

apapirovski approved these changes Feb 6, 2018
View reviewed changes

apapirovski left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM with a small nit

Comment thread lib/assert.js Outdated
return msg;
return `${key}: expected ${inspect(expected[key])}, ` +
`not ${inspect(actual[key])}`;
function compareKey(actual, expected, key, msg) {

apapirovski Feb 6, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nit: could we give this function a slightly more descriptive name since it only works for throws? Like compareExceptionKey or something?

BridgeAR Feb 6, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Done

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Feb 6, 2018

BridgeAR commented Feb 6, 2018

Copy link
Copy Markdown
Member Author

I only changed the function name since the CI and it was green before, so I do not rerun it now.

apapirovski pushed a commit to apapirovski/node that referenced this pull request Feb 9, 2018
assert: fix throws trace
cccddc5 
The current stack trace thrown in case `assert.throws(fn, object)`
is used did not filter the stack trace. This fixes it.

PR-URL: nodejs#18595
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

apapirovski commented Feb 9, 2018

Copy link
Copy Markdown
Contributor

Landed in cccddc5

apapirovski closed this Feb 9, 2018

MylesBorins commented Feb 21, 2018

Copy link
Copy Markdown
Contributor

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 8, 2018
assert: fix throws trace
6694f48 
The current stack trace thrown in case `assert.throws(fn, object)`
is used did not filter the stack trace. This fixes it.

PR-URL: nodejs#18595
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
BridgeAR mentioned this pull request Mar 8, 2018
Closed
4 tasks

BridgeAR commented Mar 8, 2018

Copy link
Copy Markdown
Member Author

Backport opened in #19230

MylesBorins pushed a commit that referenced this pull request Mar 15, 2018
assert: fix throws trace
1071799 
The current stack trace thrown in case `assert.throws(fn, object)`
is used did not filter the stack trace. This fixes it.

Backport-PR-URL: #19230
PR-URL: #18595
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
targos mentioned this pull request Mar 18, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
assert: fix throws trace
e9ac468 
The current stack trace thrown in case `assert.throws(fn, object)`
is used did not filter the stack trace. This fixes it.

Backport-PR-URL: #19230
PR-URL: #18595
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
assert: fix throws trace
1740d64 
The current stack trace thrown in case `assert.throws(fn, object)`
is used did not filter the stack trace. This fixes it.

PR-URL: nodejs#18595
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

MylesBorins commented Jul 31, 2018

Copy link
Copy Markdown
Contributor

Requested backport to 8.x in #19230

BridgeAR added a commit to BridgeAR/node that referenced this pull request Oct 2, 2018
assert: fix throws trace
dcfda69 
The current stack trace thrown in case `assert.throws(fn, object)`
is used did not filter the stack trace. This fixes it.

PR-URL: nodejs#18595
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request Oct 31, 2018
assert: fix throws trace
18071db 
The current stack trace thrown in case `assert.throws(fn, object)`
is used did not filter the stack trace. This fixes it.

Backport-PR-URL: #23223
PR-URL: #18595
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
BethGriggs mentioned this pull request Nov 12, 2018
Merged
beevelop mentioned this pull request Nov 20, 2018
Closed
BridgeAR deleted the assert-throws-stack branch April 1, 2019 23:38
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
+1 more reviewer
apapirovski apapirovski approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

assert Issues and PRs related to the assert subsystem. author ready PRs that have at least one approval, no outstanding review comments, and a CI started.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL