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

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

Closed
G-Rath opened this issue Aug 8, 2022 · 1 comment · Fixed by #1201
Closed

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

G-Rath opened this issue Aug 8, 2022 · 1 comment · Fixed by #1201

Comments

@G-Rath
Copy link
Collaborator

G-Rath commented Aug 8, 2022

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.

@github-actions
Copy link

🎉 This issue has been resolved in version 26.8.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

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