Skip to content

ci: Collect jest test file names in frontend.yml, and adjust matrix size based on the list len()#116663

Draft
ryan953 wants to merge 2 commits into
ryan953/ci-jest-matrix-dynamicfrom
ryan953/ci-jest-file-list
Draft

ci: Collect jest test file names in frontend.yml, and adjust matrix size based on the list len()#116663
ryan953 wants to merge 2 commits into
ryan953/ci-jest-matrix-dynamicfrom
ryan953/ci-jest-file-list

Conversation

@ryan953
Copy link
Copy Markdown
Member

@ryan953 ryan953 commented Jun 2, 2026

No description provided.

@ryan953 ryan953 requested a review from a team as a code owner June 2, 2026 04:08
@github-actions github-actions Bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Jun 2, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 2, 2026

📊 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 '.')"
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.

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"
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.

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.

Copy link
Copy Markdown
Contributor

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 5 potential issues.

Fix All in Cursor

❌ 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"
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.

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.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 5fbd253. Configure here.

- name: List Jest tests
id: list-jest-tests
run: |
RUNNERS=8
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.

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 }}.

Fix in Cursor Fix in Web

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 '.')"
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.

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.

Fix in Cursor Fix in Web

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"
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.

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.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 5fbd253. Configure here.

Comment thread jest.config.ts
'@sentry/toolbar': '<rootDir>/tests/js/sentry-test/mocks/sentryToolbarMock.js',
},
passWithNoTests: !!MERGE_BASE,
passWithNoTests: Boolean(JEST_TESTS?.length),
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.

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.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 5fbd253. Configure here.

@ryan953 ryan953 marked this pull request as draft June 2, 2026 05:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant