Skip to content

Commit 05ce3f9

Browse files
authored
fix(trigger-e2e): stabilize and parallelize victim scenarios (#1595)
* test(trigger-e2e): run victim scenarios concurrently Run up to four isolated trigger scenarios in parallel by default, preserve declaration-order reporting, and expose a bounded 1-8 concurrency override. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0fe13dc9-ae18-4e54-b587-a53f5afb28f2 * fix(trigger-e2e): stabilize victim scenarios Read PR labels from the dedicated ADO endpoint, remove the stale metadata dependency, and avoid Bash command substitution for unset synth variables. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0fe13dc9-ae18-4e54-b587-a53f5afb28f2 * fix(filter-ir): acquire PR labels independently Use the dedicated Azure DevOps PR labels endpoint, remove the stale metadata dependency, and harden the trigger victim's unset synth-variable handling. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0fe13dc9-ae18-4e54-b587-a53f5afb28f2
1 parent b73183a commit 05ce3f9

19 files changed

Lines changed: 208 additions & 47 deletions

File tree

docs/filter-ir.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ on `PrMetadata` being acquired first.
7777
|------|--------|-----------|------------|
7878
| `PrMetadata` | `GET pullRequests/{id}` | `PR_DATA` ||
7979
| `PrIsDraft` | `json .isDraft` from `PR_DATA` | `IS_DRAFT` | `PrMetadata` |
80-
| `PrLabels` | `json .labels[].name` from `PR_DATA` | `PR_LABELS` | `PrMetadata` |
80+
| `PrLabels` | `GET pullRequests/{id}/labels` | `PR_LABELS` | |
8181

8282
#### Iteration API-Derived
8383

@@ -175,7 +175,7 @@ Maps each field of `PrFilters` to a `FilterCheck`:
175175
| `source_branch` | `GlobMatch` | `SourceBranch` | `source-branch-mismatch` |
176176
| `target_branch` | `GlobMatch` | `TargetBranch` | `target-branch-mismatch` |
177177
| `commit_message` | `GlobMatch` | `CommitMessage` | `commit-message-mismatch` |
178-
| `labels` | `LabelSetMatch` | `PrLabels` (→ `PrMetadata`) | `labels-mismatch` |
178+
| `labels` | `LabelSetMatch` | `PrLabels` | `labels-mismatch` |
179179
| `draft` | `Equality` | `PrIsDraft` (→ `PrMetadata`) | `draft-mismatch` |
180180
| `changed_files` | `FileGlobMatch` | `ChangedFiles` | `changed-files-mismatch` |
181181
| `time_window` | `TimeWindow` | `CurrentUtcMinutes` | `time-window-mismatch` |

scripts/ado-script/src/gate/facts.test.ts

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,12 +6,14 @@ const {
66
mockReadEnvFact,
77
mockIsPipelineVarFact,
88
mockGetPullRequestById,
9+
mockGetPullRequestLabels,
910
mockGetPullRequestIterations,
1011
mockGetIterationChanges,
1112
} = vi.hoisted(() => ({
1213
mockReadEnvFact: vi.fn(),
1314
mockIsPipelineVarFact: vi.fn(),
1415
mockGetPullRequestById: vi.fn(),
16+
mockGetPullRequestLabels: vi.fn(),
1517
mockGetPullRequestIterations: vi.fn(),
1618
mockGetIterationChanges: vi.fn(),
1719
}));
@@ -23,6 +25,7 @@ vi.mock("../shared/env-facts.js", () => ({
2325

2426
vi.mock("../shared/ado-client.js", () => ({
2527
getPullRequestById: mockGetPullRequestById,
28+
getPullRequestLabels: mockGetPullRequestLabels,
2629
getPullRequestIterations: mockGetPullRequestIterations,
2730
getIterationChanges: mockGetIterationChanges,
2831
}));
@@ -72,6 +75,7 @@ describe("acquireFacts", () => {
7275
mockReadEnvFact.mockReset();
7376
mockIsPipelineVarFact.mockReset().mockReturnValue(false);
7477
mockGetPullRequestById.mockReset();
78+
mockGetPullRequestLabels.mockReset();
7579
mockGetPullRequestIterations.mockReset();
7680
mockGetIterationChanges.mockReset();
7781
});
@@ -166,18 +170,21 @@ describe("acquireFacts", () => {
166170
expect(mockGetPullRequestById).not.toHaveBeenCalled();
167171
});
168172

169-
it("derives pr_labels from cached pr_metadata", async () => {
170-
mockGetPullRequestById.mockResolvedValue({
171-
pullRequestId: 42,
172-
labels: [{ name: "ready" }, { name: "security" }, {}],
173-
});
173+
it("acquires pr_labels from the dedicated labels endpoint", async () => {
174+
mockGetPullRequestById.mockResolvedValue({ pullRequestId: 42 });
175+
mockGetPullRequestLabels.mockResolvedValue([
176+
{ name: "ready" },
177+
{ name: "security" },
178+
{},
179+
]);
174180
const { tracker } = makeTracker();
175181

176182
const facts = await acquireFacts(
177183
gateSpec([fact("pr_metadata"), fact("pr_labels")]),
178184
tracker,
179185
);
180186

187+
expect(mockGetPullRequestLabels).toHaveBeenCalledWith("project", "repo", 42);
181188
expect(facts.get("pr_labels")).toEqual(["ready", "security", ""]);
182189
});
183190

scripts/ado-script/src/gate/facts.ts

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -67,10 +67,12 @@ async function acquireOne(
6767
return md.isDraft ? "true" : "false";
6868
}
6969
case "pr_labels": {
70-
const md = facts.get("pr_metadata") as
71-
| { labels?: { name?: string }[] }
72-
| undefined;
73-
const labels = md?.labels ?? [];
70+
requireAdoCtx(ctx, "pr_labels");
71+
const labels = await adoClient.getPullRequestLabels(
72+
ctx.project,
73+
ctx.repoId,
74+
ctx.prId,
75+
);
7476
return labels.map((l) => l.name ?? "");
7577
}
7678
case "changed_files": {

scripts/ado-script/src/shared/__tests__/ado-client.test.ts

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ const { mockGitApi, mockBuildApi, mockWebApi, mockGetWebApi } = vi.hoisted(() =>
77
getBranch: vi.fn(),
88
getCommitDiffs: vi.fn(),
99
getPullRequestById: vi.fn(),
10+
getPullRequestLabels: vi.fn(),
1011
getPullRequestIterations: vi.fn(),
1112
getPullRequestIterationChanges: vi.fn(),
1213
};
@@ -29,6 +30,7 @@ vi.mock("../auth.js", () => ({
2930
import {
3031
getCommitDiffMetadata,
3132
getPullRequestById,
33+
getPullRequestLabels,
3234
getPullRequestIterations,
3335
getIterationChanges,
3436
cancelBuild,
@@ -40,6 +42,7 @@ describe("ado-client", () => {
4042
mockGitApi.getBranch.mockReset();
4143
mockGitApi.getCommitDiffs.mockReset();
4244
mockGitApi.getPullRequestById.mockReset();
45+
mockGitApi.getPullRequestLabels.mockReset();
4346
mockGitApi.getPullRequestIterations.mockReset();
4447
mockGitApi.getPullRequestIterationChanges.mockReset();
4548
mockBuildApi.updateBuild.mockReset();
@@ -57,6 +60,21 @@ describe("ado-client", () => {
5760
expect(result).toEqual({ pullRequestId: 42 });
5861
});
5962

63+
it("getPullRequestLabels calls the dedicated labels endpoint", async () => {
64+
mockGitApi.getPullRequestLabels.mockResolvedValue([
65+
{ name: "run-agent" },
66+
{ name: "security" },
67+
]);
68+
const result = await getPullRequestLabels("p", "r", 42);
69+
expect(mockGitApi.getPullRequestLabels).toHaveBeenCalledWith("r", 42, "p");
70+
expect(result).toEqual([{ name: "run-agent" }, { name: "security" }]);
71+
});
72+
73+
it("getPullRequestLabels normalizes an empty SDK response", async () => {
74+
mockGitApi.getPullRequestLabels.mockResolvedValue(null);
75+
await expect(getPullRequestLabels("p", "r", 42)).resolves.toEqual([]);
76+
});
77+
6078
it("getCommitDiffMetadata pins the target branch tip and orients the diff correctly", async () => {
6179
const source = "a".repeat(40);
6280
const target = "b".repeat(40);

scripts/ado-script/src/shared/__tests__/policy.test.ts

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import type { FactSpec } from "../types.gen.js";
77
// the call sites (matches how the compiler emits it in production).
88
const DEFAULT_DEPS: Record<string, readonly string[]> = {
99
pr_is_draft: ["pr_metadata"],
10-
pr_labels: ["pr_metadata"],
1110
changed_file_count: ["changed_files"],
1211
};
1312

@@ -57,15 +56,13 @@ describe("PolicyTracker", () => {
5756
expect(t.verdictForMissingFacts(["pr_is_draft"])).toBe("skip");
5857
});
5958

60-
it("transitive skip: pr_metadata fails skip_dependents → pr_labels skipped", () => {
59+
it("pr_metadata failure does not suppress independent pr_labels", () => {
6160
const t = new PolicyTracker([
6261
spec("pr_metadata", "skip_dependents"),
6362
spec("pr_labels", "fail_open"),
6463
]);
6564
t.recordFactFailure("pr_metadata", "API error");
66-
// Even though pr_labels is fail_open, the *skip* propagates because
67-
// its dep failed with skip_dependents. The skip dominates.
68-
expect(t.verdictForMissingFacts(["pr_labels"])).toBe("skip");
65+
expect(t.verdictForMissingFacts(["pr_labels"])).toBe("evaluate");
6966
});
7067

7168
it("transitive skip: changed_files fails skip_dependents → changed_file_count skipped", () => {

scripts/ado-script/src/shared/ado-client.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,17 @@ export async function getPullRequestById(
172172
});
173173
}
174174

175+
export async function getPullRequestLabels(
176+
project: string,
177+
repoId: string,
178+
prId: number,
179+
): Promise<Array<{ name?: string }>> {
180+
return withRetry("getPullRequestLabels", async () => {
181+
const git = await (await getWebApi()).getGitApi();
182+
return (await git.getPullRequestLabels(repoId, prId, project)) ?? [];
183+
});
184+
}
185+
175186
/**
176187
* Lists active pull requests whose `sourceRefName` matches the given
177188
* value. Used by `exec-context-pr-synth` to discover the open PR for

scripts/ado-script/src/trigger-e2e/__tests__/gate-spec.test.ts

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -28,15 +28,13 @@ describe("buildGateSpec fact derivation", () => {
2828
expect(spec.context.bypass_label).toBe("PR");
2929
});
3030

31-
it("derives pr_metadata (skip_dependents) before pr_labels (fail_open) for a labels check", () => {
31+
it("derives independent fail-open pr_labels for a labels check", () => {
3232
const spec = buildGateSpec("pull-request", [labelsCheck({ anyOf: ["run-agent"] })]);
3333
const kinds = spec.facts.map((f) => f.kind);
34-
expect(kinds).toEqual(["pr_metadata", "pr_labels"]);
34+
expect(kinds).toEqual(["pr_labels"]);
3535
const meta = Object.fromEntries(spec.facts.map((f) => [f.kind, f]));
36-
expect(meta.pr_metadata!.failure_policy).toBe("skip_dependents");
37-
expect(meta.pr_metadata!.dependencies).toEqual([]);
3836
expect(meta.pr_labels!.failure_policy).toBe("fail_open");
39-
expect(meta.pr_labels!.dependencies).toEqual(["pr_metadata"]);
37+
expect(meta.pr_labels!.dependencies).toEqual([]);
4038
});
4139

4240
it("derives pr_metadata before pr_is_draft (fail_closed) for a draft check", () => {

scripts/ado-script/src/trigger-e2e/__tests__/runner.test.ts

Lines changed: 69 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { describe, expect, it, vi } from "vitest";
22

33
import type { AdoRest } from "../../executor-e2e/ado-rest.js";
4-
import { runScenario } from "../runner.js";
4+
import { runAll, runScenario, scenarioConcurrency } from "../runner.js";
55
import { SkipError } from "../scenario.js";
66
import type { TriggerContext, TriggerScenario } from "../scenario.js";
77

@@ -142,3 +142,71 @@ describe("runScenario", () => {
142142
expect(rest.cancelBuild).not.toHaveBeenCalled();
143143
});
144144
});
145+
146+
describe("scenarioConcurrency", () => {
147+
it("defaults to four, including for an unexpanded ADO macro", () => {
148+
expect(scenarioConcurrency({})).toBe(4);
149+
expect(scenarioConcurrency({ TRIGGER_E2E_CONCURRENCY: "$(TRIGGER_E2E_CONCURRENCY)" })).toBe(4);
150+
});
151+
152+
it("accepts an explicit bounded integer", () => {
153+
expect(scenarioConcurrency({ TRIGGER_E2E_CONCURRENCY: "6" })).toBe(6);
154+
});
155+
156+
it.each(["0", "9", "1.5", "many"])("rejects invalid value %s", (value) => {
157+
expect(() => scenarioConcurrency({ TRIGGER_E2E_CONCURRENCY: value })).toThrow(
158+
"TRIGGER_E2E_CONCURRENCY",
159+
);
160+
});
161+
});
162+
163+
describe("runAll", () => {
164+
it("runs scenarios concurrently while preserving declaration order", async () => {
165+
const { ctx } = makeCtx({ result: "succeeded" });
166+
let active = 0;
167+
let maxActive = 0;
168+
169+
const delayed = (id: string, delayMs: number): TriggerScenario<string> =>
170+
scenario({
171+
id,
172+
setup: async () => {
173+
active += 1;
174+
maxActive = Math.max(maxActive, active);
175+
await new Promise((resolve) => setTimeout(resolve, delayMs));
176+
active -= 1;
177+
return id;
178+
},
179+
});
180+
181+
const results = await runAll(
182+
ctx,
183+
[delayed("first", 30), delayed("second", 5), delayed("third", 5)],
184+
2,
185+
);
186+
187+
expect(maxActive).toBe(2);
188+
expect(results.map((result) => result.id)).toEqual(["first", "second", "third"]);
189+
expect(results.every((result) => result.ok)).toBe(true);
190+
});
191+
192+
it("never exceeds the requested concurrency", async () => {
193+
const { ctx } = makeCtx({ result: "succeeded" });
194+
let active = 0;
195+
let maxActive = 0;
196+
const scenarios = Array.from({ length: 7 }, (_, index) =>
197+
scenario({
198+
id: `s-${index}`,
199+
setup: async () => {
200+
active += 1;
201+
maxActive = Math.max(maxActive, active);
202+
await new Promise((resolve) => setTimeout(resolve, 10));
203+
active -= 1;
204+
return "state";
205+
},
206+
}),
207+
);
208+
209+
await runAll(ctx, scenarios, 3);
210+
expect(maxActive).toBe(3);
211+
});
212+
});

scripts/ado-script/src/trigger-e2e/fact-catalog.gen.json

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -54,9 +54,7 @@
5454
{
5555
"kind": "pr_labels",
5656
"failure_policy": "fail_open",
57-
"dependencies": [
58-
"pr_metadata"
59-
]
57+
"dependencies": []
6058
},
6159
{
6260
"kind": "changed_files",

scripts/ado-script/src/trigger-e2e/gate-spec.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ const FACT_META: ReadonlyMap<string, FactMeta> = new Map<string, FactMeta>([
6262
["triggering_branch", { policy: "fail_closed", deps: [] }],
6363
["pr_metadata", { policy: "skip_dependents", deps: [] }],
6464
["pr_is_draft", { policy: "fail_closed", deps: ["pr_metadata"] }],
65-
["pr_labels", { policy: "fail_open", deps: ["pr_metadata"] }],
65+
["pr_labels", { policy: "fail_open", deps: [] }],
6666
["changed_files", { policy: "fail_open", deps: [] }],
6767
["changed_file_count", { policy: "fail_open", deps: ["changed_files"] }],
6868
["current_utc_minutes", { policy: "fail_closed", deps: [] }],

0 commit comments

Comments
 (0)