Skip to content

Conversation

@BridgeAR
Copy link
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

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

Copy link
Contributor

@apapirovskiapapirovski left a comment

Choose a reason for hiding this comment

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

LGTM with a small nit

lib/assert.js Outdated
returnmsg;
return`${key}: expected ${inspect(expected[key])}, `+
`not ${inspect(actual[key])}`;
functioncompareKey(actual,expected,key,msg){
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Done

@BridgeARBridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 6, 2018
@BridgeAR
Copy link
MemberAuthor

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
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 <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>
@apapirovski
Copy link
Contributor

Landed in cccddc5

@MylesBorins
Copy link
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
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 <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>
@BridgeARBridgeAR mentioned this pull request Mar 8, 2018
4 tasks
@BridgeAR
Copy link
MemberAuthor

Backport opened in #19230

MylesBorins pushed a commit that referenced this pull request Mar 15, 2018
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 <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>
@targostargos mentioned this pull request Mar 18, 2018
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
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 <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
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 <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>
@MylesBorins
Copy link
Contributor

Requested backport to 8.x in #19230

BridgeAR added a commit to BridgeAR/node that referenced this pull request Oct 2, 2018
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 <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 31, 2018
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 <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

assertIssues and PRs related to the assert subsystem.author readyPRs that have at least one approval, no pending requests for changes, and a CI started.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@BridgeAR@apapirovski@MylesBorins@jasnell@nodejs-github-bot