Skip to content

Don't set global stack trace limit to be infinite #483

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

Closed
jamestalmage opened this issue Jan 29, 2016 · 2 comments
Closed

Don't set global stack trace limit to be infinite #483

jamestalmage opened this issue Jan 29, 2016 · 2 comments
Labels
bug current functionality does not work as desired good for beginner help wanted scope:assertions

Comments

@jamestalmage
Copy link
Contributor

Infinity depth stack traces hardly seem necessary, but that is what we are currently doing.
#482 fixes that for t.plan(), which I think is important, since t.plan() generates a stack trace whether there is a test failure or not.

My remaining question is whether or not we should shorten them globally. I am not overly concerned about the performance impact of failing assertions, more so other libraries (like bluebird longStackTrace support) that might be creating overly long stacks and slowing things down.

Some investigation is needed here. If an Infinity value does not hurt the speed of a passing test suite, then let's leave it there. Otherwise we should bikeshed on how long it should be.

@vadimdemedes vadimdemedes changed the title Performance: non-infinity stack traces Limit stack traces Jan 30, 2016
@novemberborn
Copy link
Member

My remaining question is whether or not we should shorten them globally. I am not overly concerned about the performance impact of failing assertions, more so other libraries (like bluebird longStackTrace support) that might be creating overly long stacks and slowing things down.

Changing the stack trace limit may also impact code-under-test. Instead we should set it to Infinity when recording the assertion failure, and then reset it afterwards.

@novemberborn novemberborn changed the title Limit stack traces Don't set global stack trace limit to be infinite Aug 20, 2017
@novemberborn
Copy link
Member

We set the limit here:

Error.stackTraceLimit = Infinity;

Instead we should modify the AssertionError constructor to capture an infinite stack trace: https://github.com/avajs/ava/blob/dd9e8b2effe541f9f232ee622452343dac5895dd/lib/assert.js#L31:L54

We need to do the same in the getStack() method: https://github.com/avajs/ava/blob/dd9e8b2effe541f9f232ee622452343dac5895dd/lib/assert.js#L57:L61

We have some existing code for this already: https://github.com/avajs/ava/blob/dd9e8b2effe541f9f232ee622452343dac5895dd/lib/test.js#L24:L31

@novemberborn novemberborn added this to the 1.0 milestone Aug 20, 2017
@novemberborn novemberborn added scope:assertions bug current functionality does not work as desired labels Oct 25, 2017
oantoro added a commit to oantoro/ava that referenced this issue Jan 22, 2018
oantoro added a commit to oantoro/ava that referenced this issue Jan 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug current functionality does not work as desired good for beginner help wanted scope:assertions
Projects
None yet
Development

No branches or pull requests

4 participants