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

fix(reporter): keep users exact formatError result #2627

Merged
merged 3 commits into from
Apr 5, 2017

Conversation

levithomason
Copy link
Contributor

@levithomason levithomason commented Mar 25, 2017

I wrote this feature originally and haven't had time to come back and address this issue until now. See #2119 (comment).


Fixes #2626

The main purpose of the formatError function, per the docs, is to trim down the stack trace. However, no matter the return value configured, a new line is always appended. This means even when lines are removed a blank new line is still added. This results in large, blank, stack traces:

image

This PR simply returns the exact value the user has configured in their formatError function, without appending a newline to it. This allows for removing lines from the stack trace:

image

@maksimr
Copy link
Contributor

maksimr commented Mar 25, 2017

@levithomason can you fix tests

Thanks

@maksimr
Copy link
Contributor

maksimr commented Mar 25, 2017

@levithomason also it would be nice to write test on this problem

@levithomason
Copy link
Contributor Author

Sure thing. This was an in browser patch. I'll come back and give a proper update.

@levithomason
Copy link
Contributor Author

I've updated the failing test. I went to add an additional test, however, it will only duplicate the existing test as it will ensure no new line is added to the config.formatError output. Instead, I opted to change the test description to reflect the exact error result should be returned.

@maksimr
Copy link
Contributor

maksimr commented Mar 27, 2017

LGTM

@levithomason
Copy link
Contributor Author

Odd failures on CI. Can someone retrigger please?

Travis:
Just shows an infinite loading spinner for me...

AppVeyor failure

npm ERR! code EBADF
npm ERR! errno -4083
npm ERR! syscall fstat
npm ERR! EBADF: bad file descriptor, fstat
npm ERR! A complete log of this run can be found in:
npm ERR! C:\Users\appveyor\AppData\Roaming\npm-cache_logs\2017-03-27T05_29_49_919Z-debug.log
Command exited with code -4083

@levithomason
Copy link
Contributor Author

I should note that npm test passes locally.

@levithomason
Copy link
Contributor Author

@maksimr would you happen to have rights to retry tests on CI? Perhaps without cache if available?

@maksimr
Copy link
Contributor

maksimr commented Mar 29, 2017

@levithomason only on travis and I have restarted it

@levithomason
Copy link
Contributor Author

I appreciate it.

@dignifiedquire
Copy link
Member

@levithomason can you rebase onto master please, then all linting issues should be fixed and ci hopefully happy

@levithomason
Copy link
Contributor Author

Rebased and pushed. Lint and tests pass locally.

Travis looks like it will pass.

AppVeyor node 0.12

  1) launchers/process.js flow start -> timeout -> 3xrestart -> failure:
     Uncaught AssertionError: expected spy to have been called with arguments \usr\bin\browser, ["http://localhost/?id=fake-id"]
      at test\unit\launchers\process.spec.js:234:42
      at [object Object]._onTimeout (node_modules\lodash\index.js:1773:43)
  
Warning: Task "mochaTest:unit" failed.� Use --force to continue.
Aborted due to warnings.

@dignifiedquire
Copy link
Member

I restarted appveyor lets see if that helps

@levithomason
Copy link
Contributor Author

Looks like we're good to go here. Thanks for the restart.

@levithomason
Copy link
Contributor Author

@dignifiedquire bump

@dignifiedquire dignifiedquire merged commit 17c2c43 into karma-runner:master Apr 5, 2017
@dignifiedquire
Copy link
Member

Thank you :octocat:

@donaldpipowitch
Copy link

Nice

@levithomason levithomason deleted the patch-1 branch April 5, 2017 16:50
@levithomason
Copy link
Contributor Author

Awesome, thanks much. Can't wait to use this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants