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

feat(runner/stack-trace): solve issue #545 + test #1564

Merged
merged 2 commits into from Mar 9, 2015

Conversation

@a8m
Copy link
Contributor

commented Feb 22, 2015

following #545 discussion.
I added a stack-trace filter(based on @rstacruz mocha-clean module), ran make and added tests too.
If for some reason someone want to stay with full stack-trace, just run mocha with the --full-trace flag

@jancarloviray

This comment has been minimized.

Copy link

commented Feb 23, 2015

awesome. please merge this :) 👍 thanks @rstacruz.

@danielstjules

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2015

@a8m That's pretty sweet!

danielstjules:~/git/mocha (master =)
$ cat example.js
it('test', function(done) {
  done(new Error('Something went wrong!'));
});

danielstjules:~/git/mocha (master =)
$ ./bin/mocha example.js0 passing (4ms)
  1 failing

  1)  test:
     Error: Something went wrong!
      at Context.<anonymous> (/Users/danielstjules/git/mocha/example.js:2:8)
      at Test.Runnable.run (/Users/danielstjules/git/mocha/lib/runnable.js:233:15)
      at Runner.runTest (/Users/danielstjules/git/mocha/lib/runner.js:387:10)
      at /Users/danielstjules/git/mocha/lib/runner.js:470:12
      at next (/Users/danielstjules/git/mocha/lib/runner.js:312:14)
      at /Users/danielstjules/git/mocha/lib/runner.js:322:7
      at next (/Users/danielstjules/git/mocha/lib/runner.js:257:23)
      at Immediate._onImmediate (/Users/danielstjules/git/mocha/lib/runner.js:289:5)
      at processImmediate [as _immediateCallback] (timers.js:358:17)



danielstjules:~/git/mocha (master =)
$ git checkout stack-trace
Switched to branch 'stack-trace'
danielstjules:~/git/mocha (stack-trace)
$ ./bin/mocha example.js0 passing (7ms)
  1 failing

  1)  test:
     Error: Something went wrong!
      at Context.<anonymous> (example.js:2:8)
      at Test.Runnable.run (lib/runnable.js:229:15)
      at Runner.runTest (lib/runner.js:390:10)
      at lib/runner.js:473:12
      at next (lib/runner.js:315:14)
      at lib/runner.js:325:7
      at next (lib/runner.js:260:23)
      at Immediate._onImmediate (lib/runner.js:292:5)

In addition to the work here, I wonder if there'd be interest in removing parts of the trace that propagated from mocha itself. 75% of the calls in that trace are from runner.js, which isn't something I think we need to see? For example, with the test case above, I think the following output would be a lot nicer:

  1)  test:
     Error: Something went wrong!
      at Context.<anonymous> (example.js:2:8)

You'd just have to traverse the trace, bottom up, in search of the first line that wasn't a file in mocha. Once found, just drop the lines below. This way you're not hiding any information related to the user's code. I remember doing something similar with https://github.com/danielstjules/pho a while back.

@a8m

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2015

@danielstjules you get this trace because you run it from mocha itself (it's assume that is your code, and you care of it).
PTAL on the mocha filter.

@danielstjules

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2015

Exactly what I hoped for. :) +1

$ mocha example.js

  ․

  0 passing (5ms)
  1 failing

  1)  test:
     Error: Something went wrong!
      at Context.<anonymous> (example.js:2:8)

return [];
if (nodeVersion < 0x00090B) {

This comment has been minimized.

Copy link
@rstacruz

rstacruz Mar 6, 2015

Contributor

perhaps semver would be a better fit here? (semver.satisfies(process.version, '>= 0.9.11'))

This comment has been minimized.

Copy link
@a8m

a8m Mar 6, 2015

Author Contributor

maybe, but it's not part of this PR. see: commit
(I just fixed the indentation)

@rstacruz

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2015

If I may just chime in with API design input, perhaps --stack-trace is not the best choice for a name. It seems to imply that stack traces will not be shown unless it's turned on. Maybe it would be better as --full-stack-trace.

Good work!

@a8m

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2015

Good point @rstacruz,
--full-stack-trace is too long imo, maybe --full-trace could be better ?.

@a8m

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2015

update: renaming flag to: --full-trace

@danielstjules

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2015

Would you be willing to run git reset HEAD~ and git add -p to keep your feature-related changes in a separate commit from style fixes? It'll keep blame a lot cleaner. And makes the PR quicker to review. I think @rstacruz's comment on if (nodeVersion < 0x00090B) { is an example of not being sure at a glance what's feature-related vs clean up.

@a8m

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2015

@a8m

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2015

@danielstjules

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2015

Sorry to nitpick, for some reason a8m@8d90e59 still contains changes to extraGlobals, along with a bunch of other (and totally valid) style changes.

Would you mind force-pushing https://github.com/danielstjules/mocha/commits/stack-trace onto your branch? I cleaned up that second commit a bit (went from 400 line changes, to 270)

@a8m

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2015

@danielstjules

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2015

@a8m

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2015

@travisjeffery can you please take a look ?

@travisjeffery

This comment has been minimized.

Copy link
Contributor

commented Mar 9, 2015

i tried this and it didn't work for me. lemme try again...

@travisjeffery

This comment has been minimized.

Copy link
Contributor

commented Mar 9, 2015

ah nvm there we go, yup lgtm

travisjeffery added a commit that referenced this pull request Mar 9, 2015
Merge pull request #1564 from a8m/master
shorten stack traces by default, add --full-trace

@travisjeffery travisjeffery merged commit 369fb48 into mochajs:master Mar 9, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@a8m

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2015

That's awesome, thx @travisjeffery

@rstacruz

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2015

Is this something that will land in 2.3.0?

I'll update mocha-clean (the module this was based off of) to throw a warning for newer versions of mocha.

@dasilvacontin

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2015

@rstacruz supposedly. I'm very excited to see this published soon. 😄

@johannes-photobox

This comment has been minimized.

Copy link

commented Apr 23, 2015

👍

mocha-clean/brief is so much more readable!

@myndzi

This comment has been minimized.

Copy link

commented May 21, 2015

This is actually super UNhelpful in certain cases (mainly async things); I'm running Mocha programmatically and can't seem to work out how to disable this. Specifying fullTrace: true or fullStackTrace: true to the Mocha constructor or calling mocha.fullTrace() don't seem to work. Halp?

@myndzi

This comment has been minimized.

Copy link

commented May 21, 2015

Seems to be using this here: https://github.com/mochajs/mocha/blob/master/lib/runner.js#L209 -- not sure why, since the this object does not contain that option when it is enabled on mocha.

@dasilvacontin

This comment has been minimized.

Copy link
Contributor

commented May 21, 2015

Seems to be using this here: https://github.com/mochajs/mocha/blob/master/lib/runner.js#L209 -- not sure why, since the this object does not contain that option when it is enabled on mocha.

It's set here: https://github.com/mochajs/mocha/blob/master/lib/mocha.js#L429

@dasilvacontin

This comment has been minimized.

Copy link
Contributor

commented May 21, 2015

fullStackTrace: true or mocha.fullTrace() should do the trick. Else, something is going wrong.

@myndzi

This comment has been minimized.

Copy link

commented May 21, 2015

Indeed, something is going wrong. It doesn't do the trick.

@dasilvacontin

This comment has been minimized.

Copy link
Contributor

commented May 21, 2015

Can you share a Gist with sample code that reproduces the issue?

@myndzi

This comment has been minimized.

Copy link

commented May 21, 2015

Not easily, but it may be indirectly my fault; I'll have to dig deeper. I don't see why the 'this' argument would be wrong and mocha still run at all, but I'll find out. It seemed such an obvious disconnect, that I figured at first that whoever submitted the patch just didn't test the opposite case thoroughly and made a logic error.

@myndzi

This comment has been minimized.

Copy link

commented May 21, 2015

Yep, I got it sorted. It's weird to set the options and then just copy them over, seems very non-DRY. I had to monkey patch mocha.run to emit an extra event; looks like there's support in the code that will let me not have to do that anymore now. Sorry for the confusion and thanks for the prompt response.

In the opposite direction of my original comment, internal node filtering doesn't catch _stream_readable.js or net.js among other things; not sure if that is intentional. I'm pretty sure that all internal modules do not have paths, so that function could probably be simplified and made more forward-compatible too.

@benjamn

This comment has been minimized.

Copy link

commented on lib/utils.js in 558191d Jun 13, 2015

I'm seeing this function called at a time when this process refers to the variable

mocha/mocha.js

Line 6426 in f08b789

var process = {};
, not the global process object, so there is no .cwd method… any idea what to do about that?

This comment has been minimized.

Copy link

replied Jun 13, 2015

Update: I was accidentally running mocha.js as if it contained tests.

This comment has been minimized.

Copy link
Contributor

replied Jun 13, 2015

Something like process.cwd = function() { return ''; };?

This comment has been minimized.

Copy link

replied Jun 13, 2015

Yeah, I think that would have worked, though in my particular case, stackTraceFilter was called before the var process = {} line, so process was still undefined (because of var hoisting). Anyway, now that I'm not running mocha test/mocha.js, everything works just fine!

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