Skip to content
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

Merged
merged 5 commits into from
Aug 29, 2024

Conversation

ejzimmer
Copy link
Contributor

@ejzimmer ejzimmer commented Aug 6, 2024

ESLint's new config structure moves sourceType from parserOptions to languageOptions . 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

@ejzimmer ejzimmer changed the title bug: [prefer-importing-jest-globals] autofix is broken #1634 fix(prefer-importing-jest-globals): autofix is broken #1634 Aug 6, 2024
@SimenB SimenB requested a review from G-Rath August 6, 2024 22:56
Copy link
Collaborator

@G-Rath G-Rath left a 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

@ejzimmer
Copy link
Contributor Author

ejzimmer commented Aug 7, 2024

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?

@G-Rath
Copy link
Collaborator

G-Rath commented Aug 9, 2024

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 sourceType to context.parserOptions.sourceType - I think it would be best if you could provide a min. reproduction of the issue(s) you're running into on the issue, and we can go from there.

My local tests were using the latest eslint and eslint-plugin-jest with this eslint.config.js:

const pluginJest = require('eslint-plugin-jest');

// e.g. ESLINT_SOURCE_TYPE=script npx eslint . --fix
// e.g. ESLINT_SOURCE_TYPE=module npx eslint . --fix
module.exports = [
  {
    files: ['**/*.spec.js'],
    plugins: { jest: pluginJest },
    languageOptions: { sourceType: process.env.ESLINT_SOURCE_TYPE },
    rules: {
      'jest/prefer-importing-jest-globals': 'error'
    }
  }
];

@ejzimmer
Copy link
Contributor Author

ejzimmer commented Aug 12, 2024

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 parser: tsParser from the config, but breaks with it in.

@G-Rath
Copy link
Collaborator

G-Rath commented Aug 15, 2024

ok yup this is a thing with @typescript-eslint/parser, and that's why you're having trouble with a test 🙂

Here's a "correct" set of tests that fails without this patch
new RuleTester({
  parser: require.resolve('@typescript-eslint/parser'),
}).run('prefer-importing-jest-globals: typescript edition', rule, {
  valid: [],
  invalid: [
    {
      code: dedent`
        import describe from '@jest/globals';
        describe("suite", () => {
          test("foo");
          expect(true).toBeDefined();
        })
      `,
      output: dedent`
        import { describe, expect, test } from '@jest/globals';
        describe("suite", () => {
          test("foo");
          expect(true).toBeDefined();
        })
      `,
      parserOptions: { sourceType: 'module' },
      errors: [
        {
          endColumn: 7,
          column: 3,
          line: 3,
          messageId: 'preferImportingJestGlobal',
        },
      ],
    },
    {
      code: dedent`
        const {describe} = require('@jest/globals');
        describe("suite", () => {
          test("foo");
          expect(true).toBeDefined();
        })
      `,
      output: dedent`
        const { describe, expect, test } = require('@jest/globals');
        describe("suite", () => {
          test("foo");
          expect(true).toBeDefined();
        })
      `,
      parserOptions: { sourceType: 'script' },
      errors: [
        {
          endColumn: 7,
          column: 3,
          line: 3,
          messageId: 'preferImportingJestGlobal',
        },
      ],
    },
  ],
});

@ejzimmer
Copy link
Contributor Author

Oh, fantastic, thanks. I don't have any more time this week, but I'll have a look at this first thing next week

@ejzimmer
Copy link
Contributor Author

ok yup this is a thing with @typescript-eslint/parser, and that's why you're having trouble with a test 🙂
Here's a "correct" set of tests that fails without this patch

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({
Copy link
Collaborator

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:

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 }],
},
],
});

Copy link
Contributor Author

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!

@ejzimmer ejzimmer requested a review from G-Rath August 26, 2024 22:14
@G-Rath G-Rath changed the title fix(prefer-importing-jest-globals): autofix is broken #1634 fix(prefer-importing-jest-globals): support typescript-eslint parser Aug 29, 2024
Copy link
Collaborator

@G-Rath G-Rath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perfect, thanks!

@G-Rath G-Rath merged commit 307f6a7 into jest-community:main Aug 29, 2024
44 checks passed
github-actions bot pushed a commit that referenced this pull request Aug 29, 2024
## [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))
Copy link

🎉 This issue has been resolved in version 28.8.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@ejzimmer
Copy link
Contributor Author

perfect, thanks!

Thanks for all your help on this one 💜

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[prefer-importing-jest-globals] autofix is broken
3 participants