Skip to content

feat(eslint-plugin-jest): add prefer-each rule#1032

Open
eryue0220 wants to merge 2 commits into
web-infra-dev:mainfrom
eryue0220:feat/jest-prefer-each
Open

feat(eslint-plugin-jest): add prefer-each rule#1032
eryue0220 wants to merge 2 commits into
web-infra-dev:mainfrom
eryue0220:feat/jest-prefer-each

Conversation

@eryue0220
Copy link
Copy Markdown
Contributor

Summary

  • Port prefer-each from eslint-plugin-jest to rslint

Related Links

Tracking issue: #476
eslint-plugin-jest/prefer-each doc code

Checklist

  • Tests updated (or not required).
  • Documentation updated (or not required).

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces the jest/prefer-each linter rule, which recommends using .each instead of wrapping Jest test functions in manual loops. The implementation includes the rule logic, documentation, and comprehensive test cases. The reviewer identified a correctness bug in the state tracking of the rule: using a single flat slice and a simple boolean for tracking Jest calls and test cases leads to incorrect behavior with nested or sibling loops. The reviewer provided a robust solution using a loop stack and range checks to correctly attribute Jest calls to their containing loops.

Comment on lines +23 to +78
Run: func(ctx rule.RuleContext, options any) rule.RuleListeners {
var jestFnCalls []jestUtils.JestFnType
inTestCaseCall := false

recommendFn := func() string {
if len(jestFnCalls) == 1 && jestFnCalls[0] == jestUtils.JestFnTypeTest {
return "it"
}
return "describe"
}

enterForLoop := func(node *ast.Node) {
if len(jestFnCalls) == 0 || inTestCaseCall {
return
}
jestFnCalls = jestFnCalls[:0]
}

exitForLoop := func(node *ast.Node) {
if len(jestFnCalls) == 0 || inTestCaseCall {
return
}
ctx.ReportNode(node, buildPreferEachMessage(recommendFn()))
jestFnCalls = jestFnCalls[:0]
}

return rule.RuleListeners{
ast.KindForStatement: enterForLoop,
ast.KindForInStatement: enterForLoop,
ast.KindForOfStatement: enterForLoop,
rule.ListenerOnExit(ast.KindForStatement): exitForLoop,
rule.ListenerOnExit(ast.KindForInStatement): exitForLoop,
rule.ListenerOnExit(ast.KindForOfStatement): exitForLoop,
ast.KindCallExpression: func(node *ast.Node) {
jestFnCall := jestUtils.ParseJestFnCall(node, ctx)
if jestFnCall == nil {
return
}
switch jestFnCall.Kind {
case jestUtils.JestFnTypeHook,
jestUtils.JestFnTypeDescribe,
jestUtils.JestFnTypeTest:
jestFnCalls = append(jestFnCalls, jestFnCall.Kind)
}
if jestFnCall.Kind == jestUtils.JestFnTypeTest {
inTestCaseCall = true
}
},
rule.ListenerOnExit(ast.KindCallExpression): func(node *ast.Node) {
jestFnCall := jestUtils.ParseJestFnCall(node, ctx)
if jestFnCall != nil && jestFnCall.Kind == jestUtils.JestFnTypeTest {
inTestCaseCall = false
}
},
}
},
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The current implementation of prefer-each uses a single flat jestFnCalls slice to track Jest function calls across the entire file. This introduces a correctness bug when dealing with nested or sibling loops:

  1. Sibling/Nested Loops Reset State: When entering any loop, enterForLoop resets jestFnCalls to [] if it is not empty. If an outer loop contains a test case followed by a sibling loop (or a nested loop), entering that second loop will clear the outer loop's recorded Jest calls. As a result, the outer loop will not be reported, leading to false negatives.
  2. Nested Test Cases: inTestCaseCall is tracked as a simple boolean. If there are nested test cases (or nested call expressions identified as tests), exiting the inner one will set inTestCaseCall to false prematurely while still inside the outer test case.

To fix this robustly, we can maintain a stack of active loops (loopStack). Additionally, to correctly handle nested scopes and edge cases (such as expressions in computed property keys), do not simply attribute an expression to the innermost scope on the stack. Instead, walk the scope stack and check if the expression's source position is within the body-like range of each scope.

	Run(ctx rule.RuleContext, options any) rule.RuleListeners {
		type loopInfo struct {
			node        *ast.Node
			jestFnCalls []jestUtils.JestFnType
		}

		var loopStack []loopInfo
		inTestCaseCall := false

		recommendFn := func(calls []jestUtils.JestFnType) string {
			if len(calls) == 1 && calls[0] == jestUtils.JestFnTypeTest {
				return "it"
			}
			return "describe"
		}

		enterForLoop := func(node *ast.Node) {
			loopStack = append(loopStack, loopInfo{node: node})
		}

		exitForLoop := func(node *ast.Node) {
			if len(loopStack) == 0 {
				return
			}
			currentLoop := loopStack[len(loopStack)-1]
			loopStack = loopStack[:len(loopStack)-1]

			if len(currentLoop.jestFnCalls) > 0 && !inTestCaseCall {
				ctx.ReportNode(currentLoop.node, buildPreferEachMessage(recommendFn(currentLoop.jestFnCalls)))
			}
		}

		isWithinNodeRange := func(child, parent *ast.Node) bool {
			return child.Pos() >= parent.Pos() && child.End() <= parent.End()
		}

		return rule.RuleListeners{
			ast.KindForStatement:                        enterForLoop,
			ast.KindForInStatement:                      enterForLoop,
			ast.KindForOfStatement:                      enterForLoop,
			rule.ListenerOnExit(ast.KindForStatement):   exitForLoop,
			rule.ListenerOnExit(ast.KindForInStatement): exitForLoop,
			rule.ListenerOnExit(ast.KindForOfStatement): exitForLoop,
			ast.KindCallExpression: func(node *ast.Node) {
				jestFnCall := jestUtils.ParseJestFnCall(node, ctx)
				if jestFnCall == nil {
					return
				}
				switch jestFnCall.Kind {
				case jestUtils.JestFnTypeHook,
					jestUtils.JestFnTypeDescribe,
					jestUtils.JestFnTypeTest:
					if len(loopStack) > 0 && !inTestCaseCall {
						for i := len(loopStack) - 1; i >= 0; i-- {
							if isWithinNodeRange(node, loopStack[i].node) {
								loopStack[i].jestFnCalls = append(loopStack[i].jestFnCalls, jestFnCall.Kind)
								break
							}
						}
					}
				}
				if jestFnCall.Kind == jestUtils.JestFnTypeTest {
					inTestCaseCall = true
				}
			},
			rule.ListenerOnExit(ast.KindCallExpression): func(node *ast.Node) {
				jestFnCall := jestUtils.ParseJestFnCall(node, ctx)
				if jestFnCall != nil && jestFnCall.Kind == jestUtils.JestFnTypeTest {
					inTestCaseCall = false
				}
			},
		}
	}
References
  1. For linter rules that track state across nested scopes, walk the scope stack and check if the expression's source position is within the body-like range of each scope rather than attributing it directly to the innermost scope.

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.

1 participant