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

BridgeAR
Copy link
Member

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

The stack frames from .throws and .doesNotThrow got included
even though that was not intended.
A combination of try catch and common.expectsError is not necessary
as the latter already does everything on its own.
@nodejs-github-bot nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label Dec 15, 2017
Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

This might need to be semver-major.

@BridgeAR
Copy link
Member Author

@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
Copy link
Member Author

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 18, 2017
@BridgeAR
Copy link
Member Author

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

PR-URL: nodejs#17703
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Dec 19, 2017
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 29, 2017
@MylesBorins
Copy link
Contributor

This does not land cleanly on v9.x

Would someone be willing to backport?

BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 8, 2018
The stack frames from .throws and .doesNotThrow got included
even though that was not intended.

PR-URL: nodejs#17703
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 8, 2018
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Mar 8, 2018
4 tasks
@BridgeAR
Copy link
Member Author

BridgeAR commented Mar 8, 2018

Backport opened in #19230

MylesBorins pushed a commit that referenced this pull request Mar 15, 2018
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 15, 2018
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos targos mentioned this pull request Mar 18, 2018
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
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 <[email protected]>
Reviewed-By: James M Snell <[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 stack frames from .throws and .doesNotThrow got included
even though that was not intended.

PR-URL: nodejs#17703
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 31, 2018
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Nov 12, 2018
@BridgeAR BridgeAR deleted the fix-assert-stack branch April 1, 2019 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants