Skip to content

Commit d9cf05d

Browse files
chore: add --changedSince base SHA to Jest in CI (#110568)
_"Why run many unit test when few do trick?"_ – Kevin Malone, Senior Frontend Developer Changes the Frontend > Jest jobs to use Jest's [`--changedSince` option](https://jestjs.io/docs/cli#--changedsince) when possible. That has them only run test suites that are impacted by the changes in a PR based on the module dependency graph. Jest requires the commit history since the base commit, so this adds a second _"run only necessary Jest tests"_ flow in `frontend-optional.yml`. Specifically, that: 1. Increases the frontend-optional fetch-depth in PRs to `100`, which should get most PRs' commits pretty quickly 2. Tries to compute a `MERGE_BASE` using `git merge-base` * Note that this also clears `MERGE_BASE` if the PR touches any non-`static/` files. 3. If `MERGE_BASE` could be computed, passes it as a process env var to test scripts for use as Jest's `changedSince` * If `MERGE_BASE` couldn't be computed, the new _"run only necessary Jest tests"_ flow is skipped My plan (thanks to reviewer feedback) is to keep this running as a secondary, optional test flow on `master` for a couple weeks. If all seems well, we can remove the original _"run all Jest tests"_ logic and only run the _"run only necessary Jest tests"_ logic. The following PRs exercise different scales of changes: | PR | Source Files | Test Files | Interconnectivity | Ran Suites | / 1,881 suites | |---|---|---|---|---|---| | [#110624](#110624) | 0 | 1 | ⬜️ None | 1 | 0.05% | | [#111073](#111073) | 0 | 6 | ⬜️ None | 6 | 0.32% | | [#111122](#111122) | 0 | 100 | ⬜️ None | 100 | 5.3% | | [#111074](#111074) | 1 | 0 | 🟦 Low | 5 | 0.27% | | [#111075](#111075) | 4 | 0 | 🟦 Low | 7 | 0.37% | | [#111076](#111076) | 3 | 3 | 🟦 Low | 13 | 0.69% | | [#111110](#111110) | 20 | 20 | 🟦 Low | 74 | 3.9% | | [#111111](#111111) | 100 | 100 | 🟦 Low | 196 | 10.4% | | [#111119](#111119) | 1 | 1 | 🟨 Medium | 30 | 1.6% | | [#111628](#111628) | 3 | 3 | 🟨 Medium | 78 | 4.1% | | [#111629](#111629) | 10 | 10 | 🟨 Medium | 167 | 8.9% | | [#111633](#111633) | 25 | 25 | 🟨 Medium | 227 | 12.1% | | [#111652](#111652) | 100 | 100 | 🟨 Medium | 549 | 29.2% | | [#111115](#111115) | 3 | 3 | 🟧 High | 1,652 | 87.8% | | [#111083](#111083) | 20 | 20 | 🟧 High | 1,660 | 88.3% | | [#111086](#111086) | 100 | 100 | 🟧 High | 1,721 | 91.5% | | [#110568](#110568) | 0 | 0 | 🟥 Config (Full) | 1,881 | 100% | For low-connectivity changes (isolated components, feature-specific views), `--changedSince` provides pretty great savings: even 100 low-connectivity files only trigger ~10% of suites. Medium-connectivity files still scale pretty well, with 100 files hitting ~29%. High-connectivity files (widely-imported utils, core components) sadly trigger 85-90% of suites, which is inevitable given Sentry's highly connected module graph. Fixes ENG-7103
1 parent bfb775e commit d9cf05d

5 files changed

Lines changed: 146 additions & 5 deletions

File tree

.github/workflows/frontend-optional.yml

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,11 @@ jobs:
2525
testable_rules_changed: ${{ steps.changes.outputs.testable_rules_changed }}
2626
typecheckable_rules_changed: ${{ steps.changes.outputs.typecheckable_rules_changed }}
2727
frontend_all: ${{ steps.changes.outputs.frontend_all }}
28+
merge_base: ${{ steps.merge_base.outputs.merge_base }}
2829
steps:
2930
- uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7
31+
with:
32+
fetch-depth: 100
3033

3134
- name: Check for frontend file changes
3235
uses: dorny/paths-filter@0bc4621a3135347011ad047f9ecf449bf72ce2bd # v3.0.0
@@ -36,6 +39,88 @@ jobs:
3639
filters: .github/file-filters.yml
3740
list-files: shell
3841

42+
# On PRs, HEAD is the merge commit; its parents (HEAD^1, HEAD^2) are base and head.
43+
# Merge base of those two is what Jest --changedSince needs.
44+
# If merge base can't be computed or non-frontend files changed, output is empty
45+
# and the optional Jest job will be skipped entirely.
46+
- name: Get merge base for changedSince
47+
id: merge_base
48+
run: |
49+
MERGE_BASE=$(git merge-base HEAD^1 HEAD^2 2>/dev/null) || true
50+
if [ -n "$MERGE_BASE" ]; then
51+
CHANGED=$(git diff --name-only "$MERGE_BASE" HEAD^2)
52+
if echo "$CHANGED" | grep -qvE '^static/'; then
53+
echo "Non-frontend file changed — skipping optional Jest"
54+
MERGE_BASE=""
55+
else
56+
echo "Merge base: $MERGE_BASE (Jest will use --changedSince)"
57+
fi
58+
else
59+
echo "Could not compute merge base — skipping optional Jest"
60+
fi
61+
echo "merge_base=${MERGE_BASE:-}" >> "$GITHUB_OUTPUT"
62+
63+
# This job intentionally mirrors `frontend-jest-tests` in frontend.yml.
64+
# Our intent is to try it out for a few weeks and see if it's stable.
65+
frontend-jest-tests-changed-only:
66+
if: >-
67+
needs.files-changed.outputs.merge_base != '' &&
68+
(needs.files-changed.outputs.testable_rules_changed == 'true' || needs.files-changed.outputs.testable_modified == 'true')
69+
needs: [files-changed]
70+
name: Jest
71+
# If you change the runs-on image, you must also change the runner in jest-balance.yml
72+
# so that the balancer runs in the same environment as the tests.
73+
runs-on: ubuntu-24.04
74+
timeout-minutes: 30
75+
strategy:
76+
# This helps not having to run multiple jobs because one fails, thus, reducing resource usage
77+
# and reducing the risk that one of many runs would turn red again (read: intermittent tests)
78+
fail-fast: false
79+
matrix:
80+
# XXX: When updating this, make sure you also update CI_NODE_TOTAL.
81+
instance: [0, 1, 2, 3]
82+
83+
steps:
84+
- uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7
85+
name: Checkout sentry
86+
with:
87+
# PRs need history so we can compute merge base for Jest --changedSince.
88+
# 100 is an arbitrary depth that will get most reasonable PRs' commits.
89+
fetch-depth: ${{ github.event_name == 'pull_request' && '100' || '1' }}
90+
91+
- uses: ./.github/actions/setup-node-pnpm
92+
93+
- name: Download jest-balance.json
94+
id: download-artifact
95+
uses: dawidd6/action-download-artifact@ac66b43f0e6a346234dd65d4d0c8fbb31cb316e5 # v11
96+
with:
97+
workflow: 38531594 # jest-balancer.yml
98+
workflow_conclusion: success # The conclusion of the workflow we're looking for
99+
branch: master # The branch we're looking for
100+
name: jest-balance.json # Artifact name
101+
name_is_regexp: false
102+
path: tests/js/test-balancer/ # Directory where to extract artifact(s), defaults to the current directory
103+
search_artifacts: true # Search for the last workflow run whose stored the artifact we're looking for
104+
if_no_artifact_found: warn # Can be one of: "fail", "warn", "ignore"
105+
106+
- name: jest
107+
env:
108+
GITHUB_PR_SHA: ${{ github.event.pull_request.head.sha || github.sha }}
109+
GITHUB_PR_REF: ${{ github.event.pull_request.head.ref || github.ref }}
110+
# XXX: CI_NODE_TOTAL must be hardcoded to the length of strategy.matrix.instance.
111+
# Otherwise, if there are other things in the matrix, using strategy.job-total
112+
# wouldn't be correct.
113+
CI_NODE_TOTAL: 4
114+
CI_NODE_INDEX: ${{ matrix.instance }}
115+
# Disable testing-library from printing out any of of the DOM to
116+
# stdout. No one actually looks through this in CI, they're just
117+
# going to run it locally.
118+
#
119+
# This quiets up the logs quite a bit.
120+
DEBUG_PRINT_LIMIT: 0
121+
MERGE_BASE: ${{ needs.files-changed.outputs.merge_base }}
122+
run: pnpm run test-ci --forceExit
123+
39124
typescript-native:
40125
if: needs.files-changed.outputs.frontend_all == 'true'
41126
needs: files-changed

eslint.config.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -928,6 +928,7 @@ export default typescript.config([
928928
name: 'files/jest related',
929929
files: [
930930
'tests/js/jest-pegjs-transform.js',
931+
'tests/js/sentry-test/jest-environment.js',
931932
'tests/js/sentry-test/mocks/*',
932933
'tests/js/sentry-test/loadFixtures.ts',
933934
'tests/js/setup.ts',

jest.config.ts

Lines changed: 25 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,18 @@ let JEST_TESTS: string[] | undefined;
6969
// to reexec itself here
7070
if (CI && !process.env.JEST_LIST_TESTS_INNER) {
7171
try {
72-
const stdout = execFileSync('pnpm', ['exec', 'jest', '--listTests', '--json'], {
72+
const listTestArguments = ['exec', 'jest', '--listTests', '--json'];
73+
74+
if (process.env.MERGE_BASE) {
75+
console.log('MERGE_BASE detected:', process.env.MERGE_BASE);
76+
listTestArguments.push(
77+
'--changedSince',
78+
process.env.MERGE_BASE,
79+
'--passWithNoTests'
80+
);
81+
}
82+
83+
const stdout = execFileSync('pnpm', listTestArguments, {
7384
stdio: 'pipe',
7485
encoding: 'utf-8',
7586
env: {...process.env, JEST_LIST_TESTS_INNER: '1'},
@@ -108,6 +119,10 @@ function getTestsForGroup(
108119
allTests: ReadonlyArray<string>,
109120
testStats: Record<string, number>
110121
): string[] {
122+
if (allTests.length === 0) {
123+
return [];
124+
}
125+
111126
const speculatedSuiteDuration = Object.values(testStats).reduce((a, b) => a + b, 0);
112127
const targetDuration = speculatedSuiteDuration / nodeTotal;
113128

@@ -122,8 +137,13 @@ function getTestsForGroup(
122137
const tests = new Map<string, number>();
123138
const SUITE_P50_DURATION_MS = 1500;
124139

140+
const allTestsSet = new Set(allTests);
141+
125142
// First, iterate over all of the tests we have stats for.
126143
Object.entries(testStats).forEach(([test, duration]) => {
144+
if (!allTestsSet.has(test)) {
145+
return;
146+
}
127147
if (duration <= 0) {
128148
throw new Error(`Test duration is <= 0 for ${test}`);
129149
}
@@ -199,8 +219,8 @@ function getTestsForGroup(
199219
}
200220
}
201221

202-
if (!groups[nodeIndex]) {
203-
throw new Error(`No tests found for node ${nodeIndex}`);
222+
if (!groups[nodeIndex]?.length) {
223+
return ['<rootDir>/__no_tests_for_this_shard__'];
204224
}
205225
return groups[nodeIndex].map(test => `<rootDir>/${test}`);
206226
}
@@ -285,6 +305,7 @@ const config: Config.InitialOptions = {
285305
// window/cookies state.
286306
'@sentry/toolbar': '<rootDir>/tests/js/sentry-test/mocks/sentryToolbarMock.js',
287307
},
308+
passWithNoTests: !!process.env.MERGE_BASE,
288309
setupFiles: [
289310
'<rootDir>/static/app/utils/silence-react-unsafe-warnings.ts',
290311
'jest-canvas-mock',
@@ -333,8 +354,7 @@ const config: Config.InitialOptions = {
333354
*/
334355
clearMocks: true,
335356

336-
// To disable the sentry jest integration, set this to 'jsdom'
337-
testEnvironment: '@sentry/jest-environment/jsdom',
357+
testEnvironment: '<rootDir>/tests/js/sentry-test/jest-environment.js',
338358
testEnvironmentOptions: {
339359
globalsCleanup: 'on',
340360
sentryConfig: {
Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
const SentryEnvironment = require('@sentry/jest-environment/jsdom');
2+
3+
// @sentry/jest-environment mutates config.projectConfig.testEnvironmentOptions
4+
// .sentryConfig.init in-place (pushing integrations and calling Sentry.init).
5+
// When Jest runs in-band (≤1 test, e.g. via --changedSince), those mutations
6+
// create circular references that crash ScriptTransformer's config serialisation.
7+
// Deep-cloning sentryConfig isolates the mutation from the original config object.
8+
class SafeSentryEnvironment extends SentryEnvironment {
9+
/** @param {import('@jest/environment').JestEnvironmentConfig} config @param {import('@jest/environment').EnvironmentContext} context */
10+
constructor(config, context) {
11+
const sentryConfig = config.projectConfig.testEnvironmentOptions?.sentryConfig;
12+
if (sentryConfig) {
13+
config = {
14+
...config,
15+
projectConfig: {
16+
...config.projectConfig,
17+
testEnvironmentOptions: {
18+
...config.projectConfig.testEnvironmentOptions,
19+
sentryConfig: structuredClone(sentryConfig),
20+
},
21+
},
22+
};
23+
}
24+
super(config, context);
25+
}
26+
}
27+
28+
module.exports = SafeSentryEnvironment;
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
declare module '@sentry/jest-environment/jsdom' {
2+
// eslint-disable-next-line import/no-extraneous-dependencies -- transitive dep of jest
3+
import type {JestEnvironment} from '@jest/environment';
4+
5+
const SentryEnvironment: typeof JestEnvironment;
6+
export = SentryEnvironment;
7+
}

0 commit comments

Comments
 (0)