eslint-plugin-jest icon indicating copy to clipboard operation
eslint-plugin-jest copied to clipboard

[prefer-expect-assertions] expect check doesn't account for scope

Open G-Rath opened this issue 1 year ago • 0 comments

I realised this while doing #1191 and gosh is it a tough one: prefer-expect-assertions is currently checking if a function call is expect.hasAssertions or expect.assertions using non-scoped means:

const isExpectAssertionsOrHasAssertionsCall = (
  expression: TSESTree.Node,
): expression is KnownCallExpression<'assertions' | 'hasAssertions'> =>
  expression.type === AST_NODE_TYPES.CallExpression &&
  expression.callee.type === AST_NODE_TYPES.MemberExpression &&
  isSupportedAccessor(expression.callee.object, 'expect') &&
  isSupportedAccessor(expression.callee.property) &&
  ['assertions', 'hasAssertions'].includes(
    getAccessorValue(expression.callee.property),
  );

This can be fixed with this:

const isExpectAssertionsOrHasAssertionsCall2 = (
  jestFnCall: ParsedJestFnCall,
) => {
  return (
    jestFnCall.type === 'expect' &&
    jestFnCall.head.node.parent?.type ===
    AST_NODE_TYPES.MemberExpression &&
    jestFnCall.members.length === 1 &&
    ['assertions', 'hasAssertions'].includes(
      getAccessorValue(jestFnCall.members[0]),
    )
  );
};

However, there's a very subtle problem which is that the rule works by checking the first statement in the body of the function being passed to test or it - this is a problem because context.getScope() is stateful, so calling parseJestFnCall will have it use the scope of the test/it method rather than the scope of the function body the expression is in.

Because we're checking the first expression it's harder to exploit, but e.g. this won't fail:

it("returns numbers that are greater than four", function(expect) {
  expect.assertions(2);

  for(let thing in things) {
    expect(number).toBeGreaterThan(4);
  }
});

I think we should be able to solve this by switching to preemptively checking every first statement in a test/it function body and then check if is anything that means we should not report (i.e. loops) - it'll add a bit more complexity but I don't think we can resolve this bug otherwise 🤷

When I get a chance I'll probably land the initial fix first since that is still part of this and will resolve the main bug.

G-Rath avatar Aug 08 '22 20:08 G-Rath