chore(rule-tester): rethrow exceptions in RuleTester to avoid circular JSON issue #10134
Conversation
|
Thanks for the PR, @kirkwaiblinger! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
kirkwaiblinger
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10134 +/- ##
==========================================
+ Coverage 86.09% 86.18% +0.08%
==========================================
Files 428 428
Lines 14969 14988 +19
Branches 4343 4344 +1
==========================================
+ Hits 12888 12917 +29
+ Misses 1734 1725 -9
+ Partials 347 346 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
JoshuaKGoldberg
added
the
1 approval
Yeah, it really is yucky IMO 🙁 but what can you do 🤷 |
| // see also https://github.com/typescript-eslint/typescript-eslint/issues/8942 | ||
| // | ||
| // For some reason rethrowing exceptions skirts around the circular JSON error. | ||
| this.#linter = new Proxy( |
There was a problem hiding this comment.
Can't we just wrap the verify call directly?
typescript-eslint/packages/rule-tester/src/RuleTester.ts
Lines 1228 to 1235 in 0884b6a
Or failing that - directly override verify, eg
const linter = new Linter(...);
const oldVerify = linter.verify;
linter.verify = (...args) => { ... oldVerify(...args) ... };There was a problem hiding this comment.
Can't we just wrap the verify call directly?
There are three this.#linter.verify calls so I at least wanted to put them all in one codepath.
Or failing that - directly override
verify, egconst linter = new Linter(...); const oldVerify = linter.verify; linter.verify = (...args) => { ... oldVerify(...args) ... };
Yeah, that works too. I just wasn't sure if Linter was some arcane class object that depended on crazy this-context and/or getter binding stuff to work correctly (like, for example, the context parameter within the rules) or if we were safe to do that. Can switch it to that if it seems safe.
There was a problem hiding this comment.
oh yeah now that Brad mentions it, +1 to that more direct strategy
github-actions
bot
removed
the
1 approval
PR Checklist
Overview
I don't really get the circular JSON problem but looks like we can suppress it by just rethrowing the exception 🤷♂️