Skip to content

Commit 5bd18e2

Browse files
feat(flaky-history-analysis): improve flaky test log fetching and concurrency
- Updated the `flaky-history-analysis.ts` script to enhance the fetching of logs for failed unit-test jobs by introducing concurrency, significantly reducing download times. - Added a new utility function to manage concurrent promise execution, ensuring efficient handling of multiple log downloads. - Adjusted filtering logic to focus on unit-test shards, optimizing the analysis process and improving accuracy in failure reporting. - Updated `.gitignore` to include generated artifacts from the flaky unit test detection workflow.
1 parent 23cb210 commit 5bd18e2

2 files changed

Lines changed: 83 additions & 28 deletions

File tree

.github/scripts/flaky-history-analysis.ts

Lines changed: 80 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -45,11 +45,15 @@ const STATE_MARKER = '<!-- metamask-flaky-test-detection:state';
4545
const COMMENT_MARKER = '<!-- metamask-flaky-test-detection -->';
4646

4747
const WORKFLOW = 'ci.yml';
48-
// JOB_NAME is written into the output artifact as metadata only — it is not
49-
// used to filter the sampled runs. Failure-file intersection is done via FAIL
50-
// lines in the raw job logs, which covers all unit-test shards regardless of
51-
// the exact job name.
48+
// JOB_NAME is written into the output artifact as metadata. It also drives
49+
// UNIT_TEST_JOB_PREFIX: only failed jobs whose name starts with this prefix
50+
// have their logs downloaded, which eliminates large e2e/build/lint logs that
51+
// can never contain a Jest FAIL line.
5252
const JOB_NAME = 'Unit tests';
53+
// ci.yml names unit-test shards "Unit tests (1)"…"Unit tests (10)" — any
54+
// failed job with this prefix is a unit-test shard whose log may contain FAIL
55+
// lines. All other jobs (e2e, build, lint) are skipped.
56+
const UNIT_TEST_JOB_PREFIX = 'Unit tests';
5357
// How far back the historical window extends. Failures are bucketed into the
5458
// nested windows below, so a failure 5 days ago counts in both windows and
5559
// one 20 days ago counts only in the 30d bucket.
@@ -62,12 +66,14 @@ type WindowCounts = Record<WindowKey, number>;
6266
// the real bound. This cap is just a safety valve against an unbounded page
6367
// walk on an extremely busy repo.
6468
const MAX_RUNS_LISTED = 3000;
65-
// Downloading failed-step logs is the expensive part (one Octokit
66-
// downloadJobLogsForWorkflowRun call per failed job per failed run). Bound the
67-
// total and process failed runs newest-first so the short windows (7d/30d)
68-
// stay accurate even if an unusually red year exceeds the cap — only the
69-
// oldest 365d tail is then undercounted.
70-
const MAX_FAILED_LOG_FETCHES = 200;
69+
// Upper bound on failed runs whose unit-test-shard logs we download. With the
70+
// UNIT_TEST_JOB_PREFIX filter each download is small (one shard's Jest output),
71+
// so 50 is ample for a 30d window with MIN_RUNS_FOR_RATE = 5.
72+
const MAX_FAILED_LOG_FETCHES = 50;
73+
// Number of concurrent Octokit requests when fetching job lists and logs.
74+
// High enough to saturate the 30d window quickly; low enough to avoid hitting
75+
// GitHub's secondary rate limits.
76+
const DOWNLOAD_CONCURRENCY = 8;
7177
// A window needs at least this many countable runs before its failure rate is
7278
// trusted for the flaky flag, so a single early failure in a nearly-empty
7379
// window cannot flag the file on its own.
@@ -165,6 +171,27 @@ function sh(cmd: string, args: string[]): string {
165171
return execFileSync(cmd, args, { encoding: 'utf8' }).trim();
166172
}
167173

174+
// Runs fn over each item with at most `limit` concurrent promises at a time.
175+
// Preserves input order in the returned results array.
176+
async function mapWithConcurrency<T, R>(
177+
items: T[],
178+
limit: number,
179+
fn: (item: T) => Promise<R>,
180+
): Promise<R[]> {
181+
const results: R[] = new Array(items.length);
182+
let index = 0;
183+
184+
async function worker(): Promise<void> {
185+
while (index < items.length) {
186+
const i = index++;
187+
results[i] = await fn(items[i]);
188+
}
189+
}
190+
191+
await Promise.all(Array.from({ length: Math.min(limit, items.length) }, worker));
192+
return results;
193+
}
194+
168195
// Extracts the per-file state JSON block embedded in a sticky comment body.
169196
function parseStateFromComment(body: string): CommentState | null {
170197
const idx = body.indexOf(STATE_MARKER);
@@ -311,7 +338,9 @@ async function getCompletedRunsInLookback(octokit: Octokit): Promise<WorkflowRun
311338

312339
// Extracts every `FAIL <path>` line Jest emits at the top of a failed test
313340
// file's output. For each failed run, lists its jobs via Octokit, keeps only
314-
// the failed jobs, then downloads their plaintext logs one by one.
341+
// failed unit-test-shard jobs (by UNIT_TEST_JOB_PREFIX), then downloads their
342+
// plaintext logs concurrently. Skipping e2e/build/lint jobs eliminates the
343+
// dominant download cost — those logs can never contain a Jest FAIL line.
315344
// downloadJobLogsForWorkflowRun returns plaintext (not a zip archive) so no
316345
// extra dependency is needed and there is no spawnSync buffer cap.
317346
async function getFailedTestFilesForRun(
@@ -320,7 +349,7 @@ async function getFailedTestFilesForRun(
320349
repo: string,
321350
runId: number,
322351
): Promise<string[]> {
323-
let jobs: { id: number; conclusion: string | null }[];
352+
let jobs: { id: number; name: string; conclusion: string | null }[];
324353
try {
325354
jobs = await octokit.paginate(octokit.rest.actions.listJobsForWorkflowRun, {
326355
owner,
@@ -334,20 +363,30 @@ async function getFailedTestFilesForRun(
334363
return [];
335364
}
336365

337-
const failedJobs = jobs.filter((j) => j.conclusion === 'failure');
338-
const logParts: string[] = [];
339-
for (const job of failedJobs) {
340-
try {
341-
const res = await octokit.rest.actions.downloadJobLogsForWorkflowRun({
342-
owner,
343-
repo,
344-
job_id: job.id,
345-
});
346-
logParts.push(String(res.data));
347-
} catch (error) {
348-
core.warning(`downloadJobLogsForWorkflowRun ${job.id} failed: ${(error as Error).message}`);
349-
}
350-
}
366+
// Only unit-test shards ("Unit tests (N)") can contain Jest FAIL lines.
367+
const failedUnitJobs = jobs.filter(
368+
(j) => j.conclusion === 'failure' && j.name.startsWith(UNIT_TEST_JOB_PREFIX),
369+
);
370+
371+
const logParts = await mapWithConcurrency(
372+
failedUnitJobs,
373+
DOWNLOAD_CONCURRENCY,
374+
async (job) => {
375+
try {
376+
const res = await octokit.rest.actions.downloadJobLogsForWorkflowRun({
377+
owner,
378+
repo,
379+
job_id: job.id,
380+
});
381+
return String(res.data);
382+
} catch (error) {
383+
core.warning(
384+
`downloadJobLogsForWorkflowRun ${job.id} failed: ${(error as Error).message}`,
385+
);
386+
return '';
387+
}
388+
},
389+
);
351390

352391
const logOutput = logParts.join('\n');
353392
// Alternation order: try the longest extension first so "tsx" isn't
@@ -387,10 +426,23 @@ async function buildHistory(
387426
.sort((a, b) => new Date(b.createdAt).getTime() - new Date(a.createdAt).getTime())
388427
.slice(0, MAX_FAILED_LOG_FETCHES);
389428

429+
// Fetch failed-file lists for all failed runs concurrently. Each run only
430+
// downloads unit-test-shard logs (small), so the total download time is now
431+
// bounded by the slowest batch of DOWNLOAD_CONCURRENCY runs rather than by
432+
// the serial sum of all 50.
433+
const failedFilesPerRun = await mapWithConcurrency(
434+
failedRuns,
435+
DOWNLOAD_CONCURRENCY,
436+
(run) => getFailedTestFilesForRun(octokit, owner, repo, run.id),
437+
);
438+
439+
// Reduce sequentially to keep deterministic bucketing — the order of
440+
// failedRuns (newest-first) is meaningful for the log-fetch budget.
390441
const failuresByFile = new Map<string, WindowCounts>();
391-
for (const run of failedRuns) {
442+
for (let i = 0; i < failedRuns.length; i++) {
443+
const run = failedRuns[i];
392444
const keys = windowKeysForAge(ageInDays(run.createdAt));
393-
for (const file of await getFailedTestFilesForRun(octokit, owner, repo, run.id)) {
445+
for (const file of failedFilesPerRun[i]) {
394446
if (!modifiedFiles.includes(file)) continue;
395447
if (!failuresByFile.has(file)) failuresByFile.set(file, emptyWindowCounts());
396448
const counts = failuresByFile.get(file) as WindowCounts;

.gitignore

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -224,6 +224,9 @@ release-signoffs.json
224224
# Skills copied at CI time by .github/workflows/flaky-unit-test-detection.yml
225225
# from .agents/skills/mms-*/SKILL.md into ai-analyzer's skill lookup path.
226226
.ai-pr-analyzer/skills/*
227+
# Generated artifacts from the flaky-unit-test-detection workflow (Stage 1).
228+
.ai-pr-analyzer/flaky-history.json
229+
.ai-pr-analyzer/flaky-prior-state.json
227230
.cursor/rules/*
228231
!.cursor/rules/*.mdc
229232
.cursor/skills/

0 commit comments

Comments
 (0)