-
Notifications
You must be signed in to change notification settings - Fork 234
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
fix(prefer-importing-jest-globals): support typescript-eslint parser #1639
Conversation
605bc98
to
9ada166
Compare
9ada166
to
b0e52d8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would you mind exploring add a test for this? it could be hard with legacy vs flat config, so feel free bail if you can't get it working, and I can take a look later this week
I've added what I think a test case should look like, but it fails due to the type checking (both TypeScript, and the run-time check of the config). I'm not sure why though, because it looks like the test utils are converting the flat config to the old config? |
good point, though actually the flat compat tester is meant to convert legacy config to flat config when testing on v9 and our suite is passing with full coverage across all ESLint versions meaning this should already be covered. Looking into this manually, I'm not able to actually produce a bug as ESLint internally maps My local tests were using the latest
|
Minimal reproduction at https://github.com/ejzimmer/prefer-importing-jest-globals-test It looks like the typescript parser might be the issue. It works fine if I remove |
ok yup this is a thing with Here's a "correct" set of tests that fails without this patch
|
Oh, fantastic, thanks. I don't have any more time this week, but I'll have a look at this first thing next week |
cf6fec2
to
cb8c49a
Compare
cb8c49a
to
4f6cf0a
Compare
I've updated the tests, and everything looks ok, but the test coverage is failing. I assume this is because the newer version of eslint never hit the second branch of the ternary that I added? I'm not sure we can do anything about that... |
|
||
const ruleTester = new RuleTester({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure when I was working on this all of these tests were passing regardless of this change - overall, we should be retaining the existing test structure, and just adding a new typescript parser based one.
This is a good example of what I mean:
eslint-plugin-jest/src/rules/__tests__/prefer-to-contain.test.ts
Lines 214 to 228 in f969f92
new RuleTester({ | |
parser: require.resolve('@typescript-eslint/parser'), | |
}).run('prefer-to-be-null: typescript edition', rule, { | |
valid: [ | |
"(expect('Model must be bound to an array if the multiple property is true') as any).toHaveBeenTipped()", | |
'expect(a.includes(b)).toEqual(0 as boolean);', | |
], | |
invalid: [ | |
{ | |
code: 'expect(a.includes(b)).toEqual(false as boolean);', | |
output: 'expect(a).not.toContain(b);', | |
errors: [{ messageId: 'useToContain', column: 23, line: 1 }], | |
}, | |
], | |
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right, I get it now I think!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perfect, thanks!
## [28.8.1](v28.8.0...v28.8.1) (2024-08-29) ### Bug Fixes * **prefer-importing-jest-globals:** support typescript-eslint parser ([#1639](#1639)) ([307f6a7](307f6a7))
🎉 This issue has been resolved in version 28.8.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Thanks for all your help on this one 💜 |
ESLint's new config structure moves
sourceType
fromparserOptions
tolanguageOptions
. This change supports that.(I couldn't find any other references to
sourceType
in the code. It exists in tests, but they're all using the new structure)Resolves #1634