fix(scm-github): cover 18-check CI in GraphQL batch (#1969)
Raise the bounded statusCheckRollup context page to 20 so normal AO PRs include individual CI details without REST fallback, while preserving hasNextPage fallback for larger repos.
This commit is contained in:
parent
ecdf0c73ec
commit
de344c0ca7
|
|
@ -391,6 +391,15 @@ export function _resetBatchEnrichPRFailedEmittedForTesting(): void {
|
||||||
*/
|
*/
|
||||||
export const MAX_BATCH_SIZE = 25;
|
export const MAX_BATCH_SIZE = 25;
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Maximum number of status check contexts to include per PR in the GraphQL batch.
|
||||||
|
* Covers AO's current 18-check CI with small headroom while keeping batch cost bounded.
|
||||||
|
* At MAX_BATCH_SIZE (25 PRs), this yields at most 20 × 25 = 500 context nodes per query,
|
||||||
|
* well within GitHub's per-call node limits.
|
||||||
|
* Repos with more contexts still fall back safely via pageInfo.hasNextPage.
|
||||||
|
*/
|
||||||
|
export const CI_CONTEXTS_FIRST = 20;
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Check if an HTTP response contains a 304 Not Modified status.
|
* Check if an HTTP response contains a 304 Not Modified status.
|
||||||
* Handles HTTP/1.1, HTTP/2, and HTTP/2.0 status lines.
|
* Handles HTTP/1.1, HTTP/2, and HTTP/2.0 status lines.
|
||||||
|
|
@ -647,11 +656,10 @@ const PR_FIELDS = `
|
||||||
commit {
|
commit {
|
||||||
statusCheckRollup {
|
statusCheckRollup {
|
||||||
state
|
state
|
||||||
# 11 keeps per-PR node cost under budget for 25-PR batch queries
|
# Fetch enough contexts for AO's normal 18-check CI while keeping
|
||||||
# (total cost ≤5000). Repos with >11 checks lose individual check
|
# the per-PR node cost bounded. If more contexts exist, pageInfo
|
||||||
# visibility, but the rollup "state" still reflects all checks —
|
# forces REST fallback for complete individual check details.
|
||||||
# overall pass/fail detection remains correct.
|
contexts(first: ${CI_CONTEXTS_FIRST}) {
|
||||||
contexts(first: 11) {
|
|
||||||
nodes {
|
nodes {
|
||||||
... on CheckRun {
|
... on CheckRun {
|
||||||
name
|
name
|
||||||
|
|
@ -897,9 +905,9 @@ function parseCIState(statusCheckRollup: unknown): CIStatus {
|
||||||
const rollup = statusCheckRollup as Record<string, unknown>;
|
const rollup = statusCheckRollup as Record<string, unknown>;
|
||||||
const state = typeof rollup["state"] === "string" ? rollup["state"].toUpperCase() : "";
|
const state = typeof rollup["state"] === "string" ? rollup["state"].toUpperCase() : "";
|
||||||
|
|
||||||
// Map GitHub's statusCheckRollup.state to our CIStatus enum
|
// Map GitHub's statusCheckRollup.state to our CIStatus enum.
|
||||||
// This top-level state aggregates all individual checks and is
|
// This top-level state aggregates all individual checks; individual context
|
||||||
// significantly cheaper than fetching contexts (10 points vs 50+ per PR)
|
// details are parsed separately and only trusted when the page is complete.
|
||||||
if (state === "SUCCESS") return "passing";
|
if (state === "SUCCESS") return "passing";
|
||||||
if (state === "FAILURE") return "failing";
|
if (state === "FAILURE") return "failing";
|
||||||
if (state === "ERROR") return "failing";
|
if (state === "ERROR") return "failing";
|
||||||
|
|
@ -984,10 +992,10 @@ function extractPREnrichment(
|
||||||
const statusCheckRollup = commits?.nodes?.[0]?.commit?.statusCheckRollup;
|
const statusCheckRollup = commits?.nodes?.[0]?.commit?.statusCheckRollup;
|
||||||
const ciStatus = statusCheckRollup ? parseCIState(statusCheckRollup) : "none";
|
const ciStatus = statusCheckRollup ? parseCIState(statusCheckRollup) : "none";
|
||||||
|
|
||||||
// Only include ciChecks when the list is complete (no truncation).
|
// Only include ciChecks when the bounded contexts page is complete.
|
||||||
// contexts(first: 20) silently truncates PRs with >20 checks — when truncated,
|
// GitHub silently truncates larger context lists — when truncated, the failing
|
||||||
// the failing check may be missing, so we set ciChecks to undefined to force
|
// check may be missing, so set ciChecks to undefined to force the getCIChecks()
|
||||||
// the getCIChecks() REST fallback in maybeDispatchCIFailureDetails.
|
// REST fallback in maybeDispatchCIFailureDetails.
|
||||||
const contextsField = statusCheckRollup?.["contexts"] as Record<string, unknown> | undefined;
|
const contextsField = statusCheckRollup?.["contexts"] as Record<string, unknown> | undefined;
|
||||||
const pageInfo = contextsField?.["pageInfo"];
|
const pageInfo = contextsField?.["pageInfo"];
|
||||||
const contextsHasNextPage =
|
const contextsHasNextPage =
|
||||||
|
|
|
||||||
|
|
@ -1,10 +1,9 @@
|
||||||
/**
|
/**
|
||||||
* Unit tests for GraphQL batch PR enrichment.
|
* Unit tests for GraphQL batch PR enrichment.
|
||||||
*
|
*
|
||||||
* Note: The GraphQL batch query was optimized to use only the top-level
|
* Note: The GraphQL batch query uses the top-level statusCheckRollup.state
|
||||||
* statusCheckRollup.state field instead of fetching individual contexts.
|
* field for overall CI status and fetches a bounded contexts page for
|
||||||
* This reduces GraphQL API cost from ~50 points to ~10 points per PR while
|
* individual check details when the page is complete.
|
||||||
* providing the same semantic information for CI status determination.
|
|
||||||
*/
|
*/
|
||||||
|
|
||||||
import { describe, it, expect, beforeEach, vi } from "vitest";
|
import { describe, it, expect, beforeEach, vi } from "vitest";
|
||||||
|
|
@ -13,6 +12,7 @@ import { describe, it, expect, beforeEach, vi } from "vitest";
|
||||||
import {
|
import {
|
||||||
generateBatchQuery,
|
generateBatchQuery,
|
||||||
MAX_BATCH_SIZE,
|
MAX_BATCH_SIZE,
|
||||||
|
CI_CONTEXTS_FIRST,
|
||||||
parseCIState,
|
parseCIState,
|
||||||
parseReviewDecision,
|
parseReviewDecision,
|
||||||
parsePRState,
|
parsePRState,
|
||||||
|
|
@ -164,6 +164,7 @@ describe("GraphQL Batch Query Generation", () => {
|
||||||
expect(query).toContain("reviewDecision");
|
expect(query).toContain("reviewDecision");
|
||||||
expect(query).toContain("commits");
|
expect(query).toContain("commits");
|
||||||
expect(query).toContain("statusCheckRollup");
|
expect(query).toContain("statusCheckRollup");
|
||||||
|
expect(query).toContain(`contexts(first: ${CI_CONTEXTS_FIRST})`);
|
||||||
});
|
});
|
||||||
|
|
||||||
it("should use sequential numeric aliases", () => {
|
it("should use sequential numeric aliases", () => {
|
||||||
|
|
@ -277,9 +278,9 @@ describe("CI State Parsing", () => {
|
||||||
expect(parseCIState({ state: "EXPECTED" })).toBe("pending");
|
expect(parseCIState({ state: "EXPECTED" })).toBe("pending");
|
||||||
});
|
});
|
||||||
|
|
||||||
it("should parse individual contexts for detailed state", () => {
|
it("should use aggregate state even when individual contexts are present", () => {
|
||||||
// After optimization, we no longer fetch individual contexts.
|
// The top-level state provides overall CI status. Individual context
|
||||||
// The top-level state provides the same semantic information.
|
// details are parsed separately for ciChecks when the page is complete.
|
||||||
expect(parseCIState({
|
expect(parseCIState({
|
||||||
state: "PENDING",
|
state: "PENDING",
|
||||||
contexts: {
|
contexts: {
|
||||||
|
|
@ -719,10 +720,14 @@ describe("PR Enrichment Data Extraction", () => {
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
describe("MAX_BATCH_SIZE constant", () => {
|
describe("GraphQL batch sizing constants", () => {
|
||||||
it("should be defined as 25", () => {
|
it("should keep PR batches at 25 for optimal batch sizing", () => {
|
||||||
expect(MAX_BATCH_SIZE).toBe(25);
|
expect(MAX_BATCH_SIZE).toBe(25);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("should request enough CI contexts for AO's 18-check CI", () => {
|
||||||
|
expect(CI_CONTEXTS_FIRST).toBe(20);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
describe("ETag Cache", () => {
|
describe("ETag Cache", () => {
|
||||||
|
|
@ -1743,7 +1748,7 @@ describe("extractPREnrichment ciChecks", () => {
|
||||||
contexts: {
|
contexts: {
|
||||||
nodes: [
|
nodes: [
|
||||||
{ name: "check-1", status: "COMPLETED", conclusion: "FAILURE", detailsUrl: null },
|
{ name: "check-1", status: "COMPLETED", conclusion: "FAILURE", detailsUrl: null },
|
||||||
// ... 19 more checks truncated
|
// ... more checks truncated beyond CI_CONTEXTS_FIRST
|
||||||
],
|
],
|
||||||
pageInfo: { hasNextPage: true }, // list was truncated!
|
pageInfo: { hasNextPage: true }, // list was truncated!
|
||||||
},
|
},
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue