ci: Collect jest test file names in frontend.yml, and adjust matrix size based on the list len()#116663
ci: Collect jest test file names in frontend.yml, and adjust matrix size based on the list len()#116663ryan953 wants to merge 2 commits into
Conversation
📊 Type Coverage Diff✅ No new type safety issues introduced. Coverage: 93.59% |
| RUNNERS=8 | ||
| if [ "$STRATEGY" == "changedSince" ]; then | ||
| RUNNERS=1 | ||
| JEST_TESTS="$(jest --listTests --json --findRelatedTests ${{ needs.files-changed.outputs.frontend_all_files }} | jq -r '.')" |
There was a problem hiding this comment.
Bug: The STRATEGY shell variable is not exported to $GITHUB_ENV, so it's unavailable in a subsequent step, causing the --findRelatedTests optimization to be skipped.
Severity: MEDIUM
Suggested Fix
In the "Get merge base for changedSince" step, after setting the STRATEGY variable, export it to the environment by adding the line echo "STRATEGY=$STRATEGY" >> "$GITHUB_ENV". This will make the variable accessible to subsequent steps in the same job.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: .github/workflows/frontend.yml#L79
Potential issue: In the GitHub Actions workflow, the shell variable `STRATEGY` is set in
one step but is not exported to the environment using `$GITHUB_ENV`. Because each `run`
step in a job executes in a new shell, the variable is not available in the subsequent
"List Jest tests" step. This causes the conditional check `if [ "$STRATEGY" ==
"changedSince" ]` to always evaluate to false, preventing the intended
`--findRelatedTests` optimization from ever being used. As a result, the full test suite
is always listed and run, increasing CI execution time.
Did we get this right? 👍 / 👎 to inform future reviews.
| JEST_TESTS="$(jest --listTests --json)" | ||
| fi | ||
| echo "jest_test_files=${JEST_TESTS}" >> "$GITHUB_OUTPUT" | ||
| echo "jest_test_files_length=${#JEST_TESTS[@]}" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
Bug: The number of test files is incorrectly calculated as 1 because an array operator is used on a string variable, preventing dynamic scaling of test runners.
Severity: MEDIUM
Suggested Fix
To correctly count the number of files, first convert the JEST_TESTS string into a bash array. Then, use ${#array_name[@]} to get the correct count. For example: read -ra JEST_TESTS_ARRAY <<< "$JEST_TESTS" followed by echo "jest_test_files_length=${#JEST_TESTS_ARRAY[@]}" >> "$GITHUB_OUTPUT".
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: .github/workflows/frontend.yml#L84
Potential issue: The logic to dynamically scale the number of test runners is flawed.
The `JEST_TESTS` variable is a string containing a list of test files, not a bash array.
However, the code attempts to get its length using the array element count operator
`${#JEST_TESTS[@]}`. In bash, when this operator is used on a scalar string, it always
evaluates to `1`. Consequently, `jest_test_files_length` is always `1`, and the number
of runners is calculated as `ceil(1/250)`, which is also `1`. This defeats the purpose
of dynamic scaling, forcing all tests to run on a single runner.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 5 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5fbd253. Configure here.
| JEST_TESTS="$(jest --listTests --json)" | ||
| fi | ||
| echo "jest_test_files=${JEST_TESTS}" >> "$GITHUB_OUTPUT" | ||
| echo "jest_test_files_length=${#JEST_TESTS[@]}" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
Bash string length always returns 1, not array count
High Severity
${#JEST_TESTS[@]} always evaluates to 1 because JEST_TESTS is a plain string variable (containing JSON), not a bash array. In bash, applying the array-length operator ${#var[@]} on a scalar variable always returns 1. This means jest_test_files_length is always 1, causing RUNNERS to always be ceil(1/250) = 1. The dynamic matrix sizing — the stated goal of this PR — never actually works.
Reviewed by Cursor Bugbot for commit 5fbd253. Configure here.
| - name: List Jest tests | ||
| id: list-jest-tests | ||
| run: | | ||
| RUNNERS=8 |
There was a problem hiding this comment.
Shell variable $STRATEGY unavailable across workflow steps
High Severity
$STRATEGY is a shell variable set in the "Get merge base for changedSince" step's run block (lines 63/66/70), but it's referenced in the separate "List Jest tests" step's run block. Each GitHub Actions step runs in its own shell process, so $STRATEGY is always empty here. The changedSince branch is never taken, meaning the full test suite is always listed. The fix is to reference the step output via ${{ steps.merge_base.outputs.merge_base_strategy }}.
Reviewed by Cursor Bugbot for commit 5fbd253. Configure here.
| RUNNERS=8 | ||
| if [ "$STRATEGY" == "changedSince" ]; then | ||
| RUNNERS=1 | ||
| JEST_TESTS="$(jest --listTests --json --findRelatedTests ${{ needs.files-changed.outputs.frontend_all_files }} | jq -r '.')" |
There was a problem hiding this comment.
Self-referential needs reference resolves to empty
Medium Severity
${{ needs.files-changed.outputs.frontend_all_files }} is used inside the files-changed job itself. A job cannot reference its own outputs via needs — this expression resolves to empty. Additionally, frontend_all_files is not listed in the job's outputs mapping. The correct reference within the same job would be ${{ steps.changes.outputs.frontend_all_files }}. Though currently unreachable due to the $STRATEGY bug, --findRelatedTests would receive no file arguments when this path is fixed.
Reviewed by Cursor Bugbot for commit 5fbd253. Configure here.
| JEST_TESTS="$(jest --listTests --json)" | ||
| fi | ||
| echo "jest_test_files=${JEST_TESTS}" >> "$GITHUB_OUTPUT" | ||
| echo "jest_test_files_length=${#JEST_TESTS[@]}" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
Missing Node.js/pnpm setup before running jest
High Severity
The new "List Jest tests" step in the files-changed job calls jest --listTests --json directly, but this job never runs the setup-node-pnpm action. That composite action installs Node.js, pnpm, and project dependencies (including jest in node_modules). Without it, jest is not available on the PATH, and this step will fail with "command not found." Every other job that runs jest or pnpm includes setup-node-pnpm first.
Reviewed by Cursor Bugbot for commit 5fbd253. Configure here.
| '@sentry/toolbar': '<rootDir>/tests/js/sentry-test/mocks/sentryToolbarMock.js', | ||
| }, | ||
| passWithNoTests: !!MERGE_BASE, | ||
| passWithNoTests: Boolean(JEST_TESTS?.length), |
There was a problem hiding this comment.
passWithNoTests is false when test list is empty
Low Severity
When JEST_TESTS is an empty array [] (e.g., changedSince finds no related tests), Boolean(JEST_TESTS?.length) evaluates to false. Simultaneously, testMatch is set to [] on line 70, which is truthy in JavaScript, so the fallback glob on line 286 is never used. Jest finds no matching tests and fails because passWithNoTests is false. The old code used !!MERGE_BASE, which was true in this scenario, allowing the run to pass gracefully.
Reviewed by Cursor Bugbot for commit 5fbd253. Configure here.


No description provided.