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

linter: valid_describe_callback should not execute in Vitest #9060

Open
baseballyama opened this issue Feb 12, 2025 · 8 comments
Open

linter: valid_describe_callback should not execute in Vitest #9060

baseballyama opened this issue Feb 12, 2025 · 8 comments
Assignees
Labels
A-linter Area - Linter C-bug Category - Bug

Comments

@baseballyama
Copy link
Contributor

What version of Oxlint are you using?

0.15.10

What command did you run?

oxlint -c=../../../.oxlintrc.json

What does your .oxlintrc.json config file look like?

{
  "$schema": "./node_modules/oxlint/configuration_schema.json",
  "categories": {
    "correctness": "error"
  },
  "plugins": ["typescript", "unicorn", "oxc", "security", "n", "vitest"],
  "env": {},
  "settings": {},
  "rules": {
    "eqeqeq": ["error", "always", { "null": "ignore" }],
    "vitest/expect-expect": "off",
    "vitest/no-disabled-tests": "off",
    "vitest/no-conditional-expect": "off",
    "jest/no-standalone-expect": "off",
    "jest/no-export": "off",
    "jest/valid-title": "off",
    "no-document-cookie": "off",
    "no-unused-vars": "off"
  }
}

What happened?

describe('FixtureTest', async () => {
  const files = collectTsFiles('./tests/fixtures');
  for (const file of files) {
    const { config } = await import(file);

    test(file, async () => {
      await runFixtureTest(config);
    });
  }
});

oxlint reports below error.

  × eslint-plugin-jest(valid-describe-callback): No async describe callback
     ╭─[tests/fixture-test.spec.ts:116:25]
 115 │     
 116 │ ╭─▶ describe('FixtureTest', async () => {
 117 │ │     const files = collectTsFiles('./tests/fixtures');
 118 │ │     for (const file of files) {
 119 │ │       const { config } = await import(file);
 120 │ │   
 121 │ │       test(file, async () => {
 122 │ │         // eslint-disable-next-line @typescript-eslint/no-unsafe-argument
 123 │ │         await runFixtureTest(config);
 124 │ │       });
 125 │ │     }
 126 │ ╰─▶ });
     ╰────
  help: Remove `async` keyword

However, I believe that Vitest does not need to report this because it supports async functions for describe.
ref: vitest-dev/vitest#703 (comment)

@baseballyama baseballyama added A-linter Area - Linter C-bug Category - Bug labels Feb 12, 2025
@Sysix
Copy link
Collaborator

Sysix commented Feb 13, 2025

@camchenry Should we split the rules for jest / vitest?
Or we can ignore the jest case and support vitest.

@camchenry
Copy link
Member

@Sysix Could we use the framework flags to detect if we're running in a file that uses vitest? Would be nice to do something like if !using_vitest && using_async_describe { report_issue() } instead of splitting the rules just for the sake of maintenance.

@Sysix
Copy link
Collaborator

Sysix commented Feb 13, 2025

Could we use the framework flags to detect if we're running in a file that uses vitest

Framework flags are not safe, see: #8165 (comment)

I would like to have a rule extending another one, then we have only the rules object duplicated
and we can avoid remapping and creating special cases like this one.

EDIT: I am also happy with the option to go the vitest way and ignore jest

@camchenry
Copy link
Member

@Sysix could we add a config option here to change the behavior maybe? Like a boolean to allow or disallow async describe callbacks.

@Sysix
Copy link
Collaborator

Sysix commented Feb 13, 2025

@camchenry maybe.
When someone uses vitest/valid-describe-callback he does not expect that he needs to configure it.
That why my plan is to extend them with this additional (hidden) config flag :)

@baseballyama
Copy link
Contributor Author

I have enabled eslint-plugin-vitest, but oxc's message contains eslint-plugin-jest, which is confusing.

Vitest was originally designed to replace Jest and aligned its API with Jest. However, it is expected to develop more unique features for now and in the future. Sharing rules between Jest and Vitest may lead to increasing complexity.

IMO is that it would be better to create separate rules for each while extracting common parts into utility files when necessary.

@camchenry
Copy link
Member

@Sysix I feel like the simplest way to resolve this is to split the rules. I think we can only claim that a rule is Vitest+Jest compatible is if it doesn't have any significant differences, but that doesn't seem to be true in this case anymore.

@baseballyama
Copy link
Contributor Author

baseballyama commented Feb 14, 2025

@camchenry @Sysix

Can I handle this (Work to separate vitest and jest rules)? I’ve made a few oxlint rules and want to try something a bit bigger.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-linter Area - Linter C-bug Category - Bug
Projects
None yet
Development

No branches or pull requests

3 participants