feat: make detailed errors opt-in

[G-Rath]

Summary

This aims to reduce the occurence of #10577 without solving it entirely by making what I believe to be the main source (#9496) opt-in (which we can do because this is being landed in a new major).

More specifically, it changes jest-circus to not include "detailed" errors by default unless the --detailed-errors-in-results flag is passed; including these causes circular reference errors because they're actually a JavaScript reference to whatever error that got thrown in a test - on a good day, that'll be a Jest assertion error as a result of an expect failure (which don't have any circular references), but the error can be literally anything including those thrown by ESLint in its RuleTester (which includes the AST tree which has circular references) and arbitrary objects that can include functions (which cannot be serialized).

As far as I know these detailed errors are actually only being used by JetBrains IDEs and it should be trivial for those that want to actually use them to provide the new flag to have their details included, and being able to account for the tradeoffs (namely "you might get circular reference errors").

Alternatively this could be landed with the inverse default, which'd still be useful by allowing users to explicitly have those details left out so they can see what the actual error they're getting is with the understanding that their tooling might not be as effective due to the detailed errors being missing.

Test plan

umm write some tests, probably

[netlify]

✅ Deploy Preview for jestjs ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	f8eea81
🔍 Latest deploy log	https://app.netlify.com/sites/jestjs/deploys/66413a2a02266a0008bea7e3
😎 Deploy Preview	https://deploy-preview-15016--jestjs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[G-Rath]

@SimenB could you let me know if this is something you'd be ok to land in any form? and if so let me know how far you'd like to go i.e. do we still care about Jasmine, and do we need to have an extensive set of tests that tries to cover behaviour both with and without the flag and with and without circular references, etc?

[G-Rath]

oh and I tested this worked with eslint-plugin-jest - currently if you do yarn run test --watch when implementing a feature it errors, whereas with this it doesn't.

[G-Rath]

The "error with cause" snapshot change looks odd to me - not sure why this change would mean it no longer includes an entire error...?

[G-Rath]

Ok so it looks like in #13966 Jest might have started using failureDetails (which is the reporter side of errorsDetailed) internally (I can't test for sure as I've not been able to remember how to run specific e2e tests) - confirmed, if I change the test to pass in --detailed-errors-in-results it passes

What's interesting though is that PR is titled as adding support for causes to Jasmine whereas #13965 (which doesn't use failureDetails) was landed first adding it to snapshots - does that mean maybe circus doesn't need it...?

Either way it's an extra bit of annoyance but I still think having this flag is worth it

---

Ok I think what this is is that #13965 landed support for causes to jest-snapshot in the general sense, and then #13965 landed support for the runners actually passing the error details along

[SimenB]

makes sense to me, thanks!

(and sorry about the radio silence)

@segrey would this cause an issue for you?

[G-Rath]

@SimenB no worries - if you're happy to land this, I can open a YouTrack issue for supporting it.

Also not sure if it matters but should this be a global or project config? I couldn't detangle the difference especially with how the defaults work (should the value be supported in both..? Are they merged or inherited...?)

[SimenB]

Unless it makes sense to configure for some projects run by Jest and not others, it should be GlobalConfig.

[G-Rath]

Ok I think that makes sense to stay as project config then - in theory, one project might not have a risk of any circular references while others do...

I'm right though that these properties can still be configured at the top level right? And if they're set there then they're applied to all projects?

[G-Rath]

Have opened https://youtrack.jetbrains.com/issue/WEB-67108 to get feedback from the WebStorm team

[segrey]

@G-Rath Thank you very much for the change. @DmitryMakhnev will take care from the WebStorm side.

[SimenB]

Does that mean I can go ahead and merge this @DmitryMakhnev? 🙂

[DmitryMakhnev]

Hello @G-Rath and @SimenB,

Thank you so much for the changes and for keeping JetBrains IDEs in mind within the context of these changes.

I've tested the changes in WebStorm and Jest integration.
With the changes, by default, failed tests will lose the 'Click to see difference' link in WebStorm.

To maintain the default behavior, WebStorm will need to pass the --detailed-errors-in-results option to Jest unless the option is overridden. This approach has 3 issues:

1. Passing some options behind the scene can be not transparent for users.
2. Jest has strict options validation; therefore, WebStorm can't simply add the --detailed-errors-in-results option for all Jest configurations. WebStorm must check the specific version of Jest before passing the option. The checking may not work ideally in some cases.
   
3. The changes will break the default behavior for current and older versions of JetBrains IDEs. We have users who can't use the latest versions of JetBrains IDEs and experience a tangible delay in updating their IDEs.

For these reasons, at this time I would like to recommend delivering these changes with an inverse default (true by default). This will preserve the default behavior for current users while providing the option for users who know what they need to make changes.

[SimenB]

For 1& 2, we could probably read en environment variable to work around it. I wonder if a scalable (long term) solution would be to allow reporters to inject some runtime configuration. Then reporters could choose to opt in to richer (more expensive) data to enhance the DX without needing to configure jest beyond a reporter.

But 3 is deffo a worry that I don't really have a good solution for off the tje top of my head. Maybe land with inverse default as you say, figure out how these "extra" flags can be made more accessible to integrations, then flip the default in a future major?

[G-Rath]

But 3 is deffo a worry that I don't really have a good solution for off the the top of my head. Maybe land with inverse default as you say, figure out how these "extra" flags can be made more accessible to integrations, then flip the default in a future major?

The issue I have with that is I think the underlying issue has proven hard to fix, puts more work on you @SimenB as the core maintainer, and potentially drives people away from Jest - personally I consider this a flaw in my original implementation: had I known at the time that this required handling circular references (and by extension, hard to address), I'd have included this flag and made it opt-in right from the start.

Also, Jest majors are infrequent (not a reflection on Simen, I just know that Jest is big so majors are a much bigger deal 🙂).

Also also, having it enabled by default might mean people try to add more to it while the circular error still remains (we've already seen that technically with the cause support), which could cause more outcry in future if its turned off by default

The env route is what I was going to suggest and I think is the one worth taking - the breaking change notes (which in theory everyone upgrading should read) could include a callout to this; I also think worse case you could get away with changing the default from "off" -> "on" as a "fix" rather than a breaking change, if we got a bunch of feedback saying people were missing this.

---

@DmitryMakhnev on an aside, I think this is yet another good example of why it would be useful for IDEs to be providing a "CI" like env variable - I don't know if its the case everywhere, but I've been surprised the number of times IntelliJ doesn't seem to be setting any env variable that tools like Jest, Rake, ESLint, etc could use to determine that they're being run in an IDE context; I wouldn't have expected that to be much work, and the sooner it is done the sooner "everyones" IDEs are future proofed.

[DmitryMakhnev]

@SimenB,

But 3 is deffo a worry that I don't really have a good solution for off the tje top of my head. Maybe land with inverse default as you say, figure out how these "extra" flags can be made more accessible to integrations, then flip the default in a future major?

I would like to recommend not using flags in general. It would be great to resolve the main issue with the circular reference. As far as I can see, the solution was almost done in this pull request. Could you please let me know if it's possible to merge it?

[DmitryMakhnev]

@G-Rath,

@DmitryMakhnev on an aside, I think this is yet another good example of why it would be useful for IDEs to be providing a "CI" like env variable - I don't know if its the case everywhere, but I've been surprised the number of times IntelliJ doesn't seem to be setting any env variable that tools like Jest, Rake, ESLint, etc could use to determine that they're being run in an IDE context; I wouldn't have expected that to be much work, and the sooner it is done the sooner "everyones" IDEs are future proofed.

Could you please share specific cases where you would like to know that Jest is running in an IDE context?

[G-Rath]

Could you please let me know if it's possible to merge it?

It sounded like @SimenB potentially wanted more done on that which the maintainer didn't have the bandwidth for.

I'd say we'd all love to avoid this flag but this has been a hard problem to solve: #14675, #11467, #10981, #10881 - personally I'm not sure if it can be solved in a lossless way because we're dealing with completely arbitrary input.

That's why I've opted for this route, as I'm no longer sure the benefits outweigh this problem given that this happens for anyone using Jest whereas a lot of the gain is only for those running Jest tests within WebStorm (which ironically, I actually don't do that frequently).

Could you please share specific cases where you would like to know that Jest is running in an IDE context?

This case, right here.

My point is that kind of feature will always be "too late" by the time it's requested, per your point of "The changes will break the default behavior for current and older versions of JetBrains IDEs" - if we had that env now (and by extension, in a few older versions of the IDE), we'd be able to just use it and move on.

---

Two asides:

1. do you have any usage data that could be shared about the old versions of IDEs? it'd be good to know if we're talking about 5% or 50% of users
2. if we were to agree on an env variable, could WebStorm start setting it now? depending on timelines, that might help reduce the impact to "old" users since by the time Jest v30 actually releases, the first version of WebStorm that sets the variable could be a bit older

[DmitryMakhnev]

Hello @G-Rath and @SimenB,

I'm so sorry for the long silence.

I discussed these changes with colleagues and I see two options:

1. Fix circular references.
   @G-Rath said:

It sounded like @SimenB potentially wanted more done on that discussion.

However I've explored the PR changes, discussed the PR state with the contributor, and verified support of circular references and BigInt in @ungap/structured-clone. So if you agree, I would like to continue developing the PR using @ungap/structured-clone to address this request. I prefer this way.

2. Use an environment variable. I can add passing the variable for the next versions of WebStorm and JetBrains IDEs. For this, I would like to know the name of the variable.

Could you please tell me if you agree with the first option?
If not, please share the environment variable name.

Thank you so much.
Have a lovely time.

[G-Rath]

@DmitryMakhnev if JetBrains is willing to sponsor fixing the underlying issue (or at least, improving the situation hopefully significantly) aka option 1, I think everyone would be in favor of that.

re 2. and the env name, for Jest I'd probably go with something like JEST_DETAILED_ERRORS_IN_RESULTS=true

[SimenB]

Solving the underlying issue with serialization would be highly preferable!

[DmitryMakhnev]

Hello @G-Rath and @SimenB,

I've created PR #15191. Could you please review it?

Thank you so much.
Have a lovely time.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.