avajs / ava

Node.js test runner that lets you develop with confidence 🚀
MIT License
20.74k stars 1.41k forks source link

Error message gets swallowed when Symbol.toStringTag is implemented #3161

Closed jhnns closed 1 year ago

jhnns commented 1 year ago

Hello 👋

First of all: thank you for ava ❤️

I've encountered the following problem with the popular Prisma ORM library:

When code throws an instance of an Error class that implements Symbol.toStringTag, the error message is not displayed in the console.

Example:

import test from "ava";

class SomeError extends Error {
  get [Symbol.toStringTag]() {
    return "SomeError";
  }
}

test("Invisible error message", (t) => {
  throw new SomeError("You can't see me");
});

Produces the following console output:

  Error thrown in test:

  SomeError {}

In Prisma, when an object does not match a given schema, they throw a PrismaClientValidationError that implements the Symbol.toStringTag. In that case, the console output is just:

Rejected promise returned by test. Reason:

  PrismaClientValidationError {
    clientVersion: '4.8.0',
  }

The error message would contain the useful information about what went actually wrong. So that's pretty confusing 😞


const config = {
  files: ["src/**/*.test.ts"],
  serial: true,
  failFast: true,
  timeout: "30s",
  extensions: {
    ts: "module",
  },
};

export default config;
npx ava --version
5.1.0
novemberborn commented 1 year ago

This is surprisingly complicated…

Errors are a bit special, in that name and message are not enumerable but we do want to show them:

> Object.getOwnPropertyNames(new Error())
[ 'stack' ]

The formatter library only does the special formatting if the string tag is Error. But, if I'm not mistaken, we only end up taking that route because AVA itself may not think it has an error:

https://github.com/avajs/ava/blob/841526144eb5529d057cb8b390a0b3bad0dda5eb/lib/serialize-error.js#L148-L153

My guess (I haven't verified) is that this code gets tripped up: https://github.com/mk-pmb/is-error-js

If that's the case we have a good chance of finding an alternative within AVA itself.

Improving the formatter library or how it hooks into AVA is more of a challenge.

jhnns commented 1 year ago

My guess (I haven't verified) is that this code gets tripped up: mk-pmb/is-error-js

I'm afraid that guess is not correct 😞. In the given example (and also in Prisma) the error object extends Error. is-error checks with instanceof which should work as expected here. I've added console.log(isError(error)) which evaluates to true.

The problem is that the error object is not detected as error in concordance.

I know that checking for the string tag works more reliably when it comes to different execution contexts, but maybe it should also check for instanceof? Of course, this would not work for cases where toStringTag is overwriten and the error is from a different execution context. In that case only duck typing would help...

novemberborn commented 1 year ago

We can probably tweak the heuristics, e.g. check if the string tag ends in Error. Not sure how much bitrot has occurred since the last update in that repo though.

jhnns commented 1 year ago

We can probably tweak the heuristics, e.g. check if the string tag ends in Error

Ha, I was also thinking of that 😁. Personally I think it's a trade off that is ok.

novemberborn commented 1 year ago

Only downside I can think of is it'd break existing snapshots. Maybe it's fine, we'd expect the name and message to be included.

jhnns commented 1 year ago

Ah, ok, concordance is also used to format objects for snapshots? I think the change would be an improvement but I understand that it could be considered a breaking change.

novemberborn commented 1 year ago

Yea… we do have a good opportunity soon with Node.js 14 reaching EOL. There are other versioning opportunities to not break snapshots but that's probably more trouble than it's worth.

jhnns commented 1 year ago

Sounds good. Let me know if you want any help, like a PR at concordance :)

novemberborn commented 1 year ago

@jhnns that'd be nice! I don't have a lot of time these days and getting back into older code needs a good chunk of it…

novemberborn commented 1 year ago

Closing since this isn't something we can fix within AVA itself.