jest-community / eslint-plugin-jest

ESLint plugin for Jest
MIT License
1.12k stars 228 forks source link

False positive for `no-standalone-expect` when using `expect.hasAssertions` in `beforeEach` hook? #1606

Open SimonSchick opened 1 month ago

SimonSchick commented 1 month ago

As stated in https://github.com/jest-community/eslint-plugin-jest/blob/main/docs/rules/no-standalone-expect.md#rule-details hasAssertions should NOT trigger this rule but it does currently?

image image

Eslint version: 8.57.0 Plugin version: 28.6.0

G-Rath commented 1 month ago

huh yeah good point - that section of the doc seems weird in general:

This rule aims to eliminate expect statements that will not be executed. An expect inside of a describe block but outside of a test or it block or outside a describe will not execute and therefore will trigger this rule. It is viable, however, to have an expect in a helper function that is called from within a test or it block so expect statements in a function will not trigger this rule.

Statements like expect.hasAssertions() will NOT trigger this rule since these calls will execute if they are not in a test block.

  1. the execution of any code is dependent on the conditions around it, so it's not correct to say "eliminate expect statements that will not be executed" if we're also saying "Statements like expect.hasAssertions() will NOT trigger this rule since these calls will execute if they are not in a test block.", as that's a contradiction
  2. the helper function thing sounds at odds with the fact that we have additionalTestBlockFunctions too
  3. we are reporting on expect.hasAssertions (as you've pointed out)

Ironically, I explicitly changed the rule to only report on those methods in #1191 after we rolled out the new Jest function "finder" which found a few false positives in the rule.

I think overall the rule should aim to report uses of expect assertions outside of test and it statements, along with in functions that match additionalTestBlockFunctions; the only thing I'm on the fence about is if it should care about hooks - we could either:

  1. just allow any expect.<method> calls outside of test and it
  2. allow most expect.<method> calls outside of test and it, but require hasAssertions to be either in beforeAll or beforeEach
  3. allow most expect.<method> calls outside of test and it, but require hasAssertions and assertions to be either in beforeAll or beforeEach
SimonSchick commented 1 month ago

I'd personally gravitate towards 2, it might also be useful to have something alike to jest/expect-expect assertFunctionNames where we would allow-list functions to be called individually or by pattern if needed.