Jest, ValidationErrors and stringifying

[brudil]

Creating this as a discussion as it's perhaps debatable if Joist should go out of its way to support this.

Essentially, Jest w/ workers posts the errors of test back to the root process using JSON.stringify. ValidationErrors holds entities which might contain circular references which JSON.stringify throws up on.

Unfortunetly a basic toJSON method on ValidationErrors doesn't fix this, a la:

toJSON() {
  return `<ValidationErrors> ${this.message}`
}

It just wasn't getting picked up... but as a property it does work; so I suspect Jest is spreading the object prior to stringifcation.

export class ValidationErrors extends Error {
  ...
  constructor(messageOrErrors: ValidationError[] | string) {
    super(typeof messageOrErrors === "string" ? messageOrErrors : errorMessage(messageOrErrors));
    this.errors = typeof messageOrErrors === "string" ? [] : messageOrErrors;
    this.toJSON = () => <ValidationErrors> ${this.message}`;
  }
}

I'm happy to PR this if it's something you're happy to have included (the property bit is annoying as this is probably a Jest only thing), but from a dx point of view I'm suprised others haven't hit this (perhaps it's something weird I'm doing) otherwise we have a workaround in extending EntityManager in our tests and catching and rethrowing validation errors without entities.

[brudil]

Percolating on this a bit more - yes I am doing something weird. So this isn't going to be a standard issue to have.

For a bit of context, we have our own in-house library for factories. Pretty simular to Laravel's API (https://laravel.com/docs/10.x/eloquent-factories) Too many tests use these for us to initially move over to Joist's factories, so I've been reworking our in-house factories to build Joist entities. I clearly haven't finished that work :D

That's why we're experiencing ValidationErrors at flush - because of a lack of type-safety at em.create (within factories) - whereas in normal usage, ValidationErrors will be prevented as a TS type issue.

Happy to close.

[stephenh]

Yeah, we've ran into this as well, and are currently using the old jest-jasmine2 runner in our jest-config to avoid this issue:

  // We reverted back to the old jest-jasmine2 runner because of this upstream issue:
  // https://github.com/facebook/jest/issues/11958
  // If it gets resolved, feel free to remove this to switch back to jest-circus.
  testRunner: "jest-jasmine2",

whereas in normal usage, ValidationErrors will be prevented as a TS type issue.

For the not-type-safe em.create errors that is true, but we also have complex validation rules that fail all the time, i.e. either transiently while flushing out a test case/implementation for the validation rule under test, or permanently if we do something like await expect(em.flush()).rejects.toThrow("Make sure we don't allow this to happen").

It is probably worth keeping an issue/discussion open for this, b/c it is annoying, and hopefully we can find a more kosher way around it than pinning to jest-jasmine2. Probably worth an FAQ/note in the docs as well...

[brudil]

Ahh, yes! I didn't even consider additional rules.

If it's affecting others, the above toJSON property does work. Or if not desired on the ValidationErrors instance itself, I suspect a getter + keeping the errors in an object with a toJSON would also kept it at arms length from these issues.

[stephenh]

Oh huh, the toJSON override actually avoids the issue now?

In that bug report, we specifically tried implementing ValidationError.toJSON with a ~dummy impl (we've done similar on EntityManager just as a precaution), and at the time it didn't work.

@blimmer see ^, but it sounds like maybe jestjs/jest#11958 has been fixed since you filed it?

If so, yep, @brudil a PR that adds a ValidationErrors.toJSON would be great.

[brudil]

Interesting issue linked! I suspect "clone without prototype" is why toJSON as a property works but not as method.

Will get up a PR!

[blimmer]

Should be fairly easy to test! I have a sample repo attached to the linked issue you're welcome to give a shot. I'm AFK for a bit 😃

[brudil]

Yup - thought I removed all references to that domain. How foolish I was to not enable autorenew…

Setting to toJSON as a property seems to work in your repro as well!

Moving the toJSON method to be set in the constructor

    this.toJSON = () => { safe: 'object' }

fwiw; not intentionally locked down so that's a bit odd.

[brudil]

This landed in v1.89.2