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.

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

assert: fix .throws and .doesNotThrow stack frames #17703

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
7 participants
@BridgeAR
Copy link
Member

BridgeAR commented Dec 15, 2017

All assert functions suppress their own function name from showing up in the stack trace.
That was not the case with throws and doesNotThrow.

On top of that I refactored two common.expectsError cases that were less than ideal. I can open a new PR for that if that is requested, but I stumbled upon those when looking for a good place to add a new test, so I thought it would be fine to include it in this PR.

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, test

BridgeAR added some commits Dec 15, 2017

assert: fix throws and doesNotThrow stack frames
The stack frames from .throws and .doesNotThrow got included
even though that was not intended.
test: refactor common.expectsError
A combination of try catch and common.expectsError is not necessary
as the latter already does everything on its own.
@TimothyGu
Copy link
Member

TimothyGu left a comment

This might need to be semver-major.

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Dec 16, 2017

@TimothyGu theoretically someone could rely on that behavior but stack frames change relatively often without it being semver-major. It could be argued that this is specifically semver-major because it is the assert module but I do not really see that.

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Dec 18, 2017

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Dec 19, 2017

Landed in dc2e266, d9171fe

@BridgeAR BridgeAR closed this Dec 19, 2017

BridgeAR added a commit to BridgeAR/node that referenced this pull request Dec 19, 2017

assert: fix throws and doesNotThrow stack frames
The stack frames from .throws and .doesNotThrow got included
even though that was not intended.

PR-URL: nodejs#17703
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

BridgeAR added a commit to BridgeAR/node that referenced this pull request Dec 19, 2017

test: refactor common.expectsError
A combination of try catch and common.expectsError is not necessary
as the latter already does everything on its own.

PR-URL: nodejs#17703
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@addaleax addaleax removed the author ready label Dec 29, 2017

@MylesBorins

This comment has been minimized.

Copy link
Member

MylesBorins commented Jan 9, 2018

This does not land cleanly on v9.x

Would someone be willing to backport?

msoechting added a commit to hpicgs/node that referenced this pull request Feb 5, 2018

assert: fix throws and doesNotThrow stack frames
The stack frames from .throws and .doesNotThrow got included
even though that was not intended.

PR-URL: nodejs#17703
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

msoechting added a commit to hpicgs/node that referenced this pull request Feb 5, 2018

test: refactor common.expectsError
A combination of try catch and common.expectsError is not necessary
as the latter already does everything on its own.

PR-URL: nodejs#17703
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

msoechting added a commit to hpicgs/node that referenced this pull request Feb 7, 2018

assert: fix throws and doesNotThrow stack frames
The stack frames from .throws and .doesNotThrow got included
even though that was not intended.

PR-URL: nodejs#17703
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

msoechting added a commit to hpicgs/node that referenced this pull request Feb 7, 2018

test: refactor common.expectsError
A combination of try catch and common.expectsError is not necessary
as the latter already does everything on its own.

PR-URL: nodejs#17703
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 8, 2018

assert: fix throws and doesNotThrow stack frames
The stack frames from .throws and .doesNotThrow got included
even though that was not intended.

PR-URL: nodejs#17703
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 8, 2018

test: refactor common.expectsError
A combination of try catch and common.expectsError is not necessary
as the latter already does everything on its own.

PR-URL: nodejs#17703
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@BridgeAR BridgeAR referenced this pull request Mar 8, 2018

Closed

[9.x backport] assert & util PRs #19230

4 of 4 tasks complete
@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Mar 8, 2018

Backport opened in #19230

MylesBorins added a commit that referenced this pull request Mar 15, 2018

assert: fix throws and doesNotThrow stack frames
The stack frames from .throws and .doesNotThrow got included
even though that was not intended.

Backport-PR-URL: #19230
PR-URL: #17703
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins added a commit that referenced this pull request Mar 15, 2018

test: refactor common.expectsError
A combination of try catch and common.expectsError is not necessary
as the latter already does everything on its own.

Backport-PR-URL: #19230
PR-URL: #17703
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@targos targos referenced this pull request Mar 18, 2018

Merged

v9.9.0 proposal #19428

MylesBorins added a commit that referenced this pull request Mar 20, 2018

assert: fix throws and doesNotThrow stack frames
The stack frames from .throws and .doesNotThrow got included
even though that was not intended.

Backport-PR-URL: #19230
PR-URL: #17703
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins added a commit that referenced this pull request Mar 20, 2018

test: refactor common.expectsError
A combination of try catch and common.expectsError is not necessary
as the latter already does everything on its own.

Backport-PR-URL: #19230
PR-URL: #17703
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins

This comment has been minimized.

Copy link
Member

MylesBorins commented Jul 31, 2018

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 and doesNotThrow stack frames
The stack frames from .throws and .doesNotThrow got included
even though that was not intended.

PR-URL: nodejs#17703
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

BridgeAR added a commit to BridgeAR/node that referenced this pull request Oct 2, 2018

assert: fix throws and doesNotThrow stack frames
The stack frames from .throws and .doesNotThrow got included
even though that was not intended.

PR-URL: nodejs#17703
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins added a commit that referenced this pull request Oct 31, 2018

assert: fix throws and doesNotThrow stack frames
The stack frames from .throws and .doesNotThrow got included
even though that was not intended.

Backport-PR-URL: #23223
PR-URL: #17703
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@BethGriggs BethGriggs referenced this pull request Nov 12, 2018

Merged

v8.13.0 proposal #23974

@BridgeAR BridgeAR deleted the BridgeAR:fix-assert-stack branch Apr 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.