doesNotThrow swallows stacktrace info

[olsonpm]

Given a method that possibly throws an exception, I get the following stacktrace by wrapping it inside doesNotThrow:

AssertionError: expected [Function] to not throw an error but 'AssertionError: expected [<...>] to be a function' was thrown
at Context.<method> (<file>:<line>:<char>)

if I just run the function without wrapping it

<Class> <method>:
AssertionError: expected [<...>] to be a function
  at <Class>.<method> (<file>:<line>:<char>) <----------This line is missing from above
  at <Context>.<method> (<file>:<line>:<char>)

[keithamus]

Thanks for the report @olsonpm! Do you have any idea what might be causing it? It'd be sweet if you could set up a reduced test case to prove it out. If you have an idea of what is causing it, feel free to make a PR to fix it.

[olsonpm]

I appreciate the response - but honestly I won't have time to look into the problem.

[dasilvacontin]

I see what happened. The output is actually correct. I reproduced it with this simple code:

require('chai').should()

var f = function throwingFunction() {
    throw new Error
}

f.should.not.throw() // comment this line
f()

In the first case you are seeing the stack trace of the AssertionError, which is generated (in my case) when executing .throw().

In the second case you are seeing the stack trace of the actual error thrown by the tested function.

I believe that showing the stack trace of the actual thrown error would be more useful, since you are also getting info about where the actual error was thrown.

If you want to change this behaviour, I'd be willing to prepare a PR. Cheers!

[olsonpm]

Good to know. Thanks for taking the time to look into that and sharing

[dasilvacontin]

You're welcome!

[keithamus]

Many thanks @dasilvacontin for taking the time to figure this issue out. Your idea sounds like something that will benefit chai users so I'd love to see an attempt at a PR!

The throw assertion is quite a big method, it lives in assertions.js#L1115-1230. I think the way to handle this would be to capture the stack trace of the caught error right after the catch on L1146 and try to emit the correct line for each assertion. It'll be quite a bit of work because throw() takes a lot of different types. A good PR would also have test coverage for each of these types - so once again quite a bit of work. As always though, PRs mean prizes and you'll go home with an awesome place on our hall of fame 😄

[dasilvacontin]

You're welcome!

Ouch, a hard fix. I'll probably won't have time to work on this for a while, maybe a month or so; a couple of important bugs surfaced in mocha, among other things. But I'll keep this on my 'wanted' list, just in case. :)

[keithamus]

At the risk of doing a bit of self promotion - let me know if I can help out anywhere with the Mocha project. I use it every day, and so if it needs some love I'm sure I can spend some time to get stuck in!

[dasilvacontin]

Sure, if I see an issue that could be a good-first one, I'll let you know. :)

[silkentrance]

we need this ASAP! and likewise so with throws()... which is also hiding the stack trace, e.g.

  1) YpoLexer #tokenize() must fail on invalid input:
     AssertionError: expected [Function: tc] to throw 'ParseError' but 'TypeError: Cannot read property \'regex\' of undefined' was thrown
      at Context.<anonymous> (lexer-test.es:86:13)

With that information from mocha I am having a hard time figuring out where the actual error is located in.

[keithamus]

@lucasfcosta this is kind of related to #683 - it'd be great if you could check to see if #683 fixes this one or not.

[meeber]

@keithamus This one isn't closed by it. There's actually a couple posts between @lucasfcosta and I near the bottom of #683 where we list which ones can be closed. :D

[lucasfcosta]

@keithamus These are the posts in which we talk about that.
I also would like to point out that we may keep #635 open too just to keep it on sight. I'll take some time to do polishments on the assert interface today after lunch.

My PR (#683) doesn't fix this one. We will need to spend some more time investigating this one.

[keithamus]

Thanks everyone for input on this issue!

We've added this to our Roadmap https:/chaijs/chai/projects/2! We'll be releasing chai 5 soon, but for now I'll close this issue because it is tracked on our roadmap.