Skip to content

Error serialization must handle non-errors better #1562

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
novemberborn opened this issue Oct 23, 2017 · 4 comments
Closed

Error serialization must handle non-errors better #1562

novemberborn opened this issue Oct 23, 2017 · 4 comments
Labels
bug current functionality does not work as desired help wanted scope:internals

Comments

@novemberborn
Copy link
Member

serialize-error currently does not guard against being called with a null or undefined value.

import test from 'ava'

test.cb(t => {
  setImmediate(() => {throw null})
  setImmediate(t.end)
})

results in:

npx ava

  1 passed
  1 exception

  Uncaught Exception
  Error: Failed to serialize uncaught exception
    at process.on.exception (/private/var/folders/2_/qczp184x76b2nl034sq5hvxw0000gn/T/tmp.dcvW5BUyA5/node_modules/ava/lib/test-worker.js:93:15)
    at emitOne (events.js:115:13)
    at process.emit (events.js:210:7)
    at process._fatalException (bootstrap_node.js:374:26)

Similarly, if a primitive is thrown, the output is terrible:

import test from 'ava'

test.cb(t => {
  setImmediate(() => {throw 5})
  setImmediate(t.end)
})
npx ava

  1 passed
  1 exception

  Uncaught Exception
  Threw non-error: {"avaAssertionError":false,"source":null,"object":5,"type":"exception","file":"test.js"}

The same goes when throwing objects:

import test from 'ava'

test.cb(t => {
  setImmediate(() => {throw new Date()})
  setImmediate(t.end)
})
npx ava

  1 passed
  1 exception

  Uncaught Exception
  Threw non-error: {"avaAssertionError":false,"source":null,"object":{},"type":"exception","file":"test.js"}

If the error doesn't seem to be an error we should format it and print that instead.

@novemberborn novemberborn added bug current functionality does not work as desired help wanted labels Oct 23, 2017
@novemberborn novemberborn added this to the 1.0 milestone Oct 23, 2017
@sindresorhus
Copy link
Member

WIth:

throw 1;

I would print:

Threw non-error: 1
YOU SHOULD NOT THROW NON-ERRORS. GO THINK ABOUT WHAT YOU DID.

Jokes aside, I'm serious about including the first sentence in some form. People should learn. The fact that it's even allowed in JS is very JS, but also very annoying.

@novemberborn
Copy link
Member Author

Here we're dealing with errors that are not handled in the test or by assertions. They could even be bugs (throw err where err is an undefined variable). I don't think we need to proselytize in the uncaught exception handler. That's what t.throws() is for.

@sindresorhus
Copy link
Member

Ok

@novemberborn novemberborn removed this from the 1.0 milestone Oct 25, 2017
@novemberborn
Copy link
Member Author

This was fixed in #1722.

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 help wanted scope:internals
Projects
None yet
Development

No branches or pull requests

2 participants