Skip to content

Commit d46a3ff

Browse files
authored
[bots] Checking for approved workflow runs: handle runs being startup failure (#6329)
If all the runs are startup failure, the function would return true. However, this can hide the need for those runs to be approved. This might result in runs being rejected when they shouldn't, but I doubt an actual PR is going to mangle workflows and still want CI Err on the side of caution by refusing if any job has a startup failure, even if there are approved runs Example: pytorch/pytorch#147774 (I am hash update bot, its a poorly named account since I was originally going to use for something else and was the fastest account I could get right now to test this) There is a startup_failure conclusion, but depending on how you mess up the workflow syntax, you can either get failure or startup_failure (https://github.com/pytorch/pytorch/pull/148016/files), and it seems like if it is going to be a startup_failure if it runs, the workflow doesn't show up at all when you query about the commit
1 parent 7151fb5 commit d46a3ff

File tree

2 files changed

+101
-1
lines changed

2 files changed

+101
-1
lines changed

torchci/lib/bot/utils.ts

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -270,7 +270,14 @@ export async function hasApprovedPullRuns(
270270
if (pr_runs == null || pr_runs?.length == 0) {
271271
return false;
272272
}
273-
return pr_runs.every((run) => run.conclusion != "action_required");
273+
return !pr_runs.some(
274+
(run) =>
275+
run.conclusion === "action_required" ||
276+
// See https://github.com/pytorch/test-infra/pull/6329 about difference
277+
// between these two
278+
run.conclusion === "startup_failure" ||
279+
(run.conclusion === "failure" && run.created_at == run.updated_at)
280+
);
274281
}
275282

276283
export async function isFirstTimeContributor(

torchci/test/utils.test.ts

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
import { hasApprovedPullRuns } from "lib/bot/utils";
2+
import nock from "nock";
3+
import { Probot } from "probot";
4+
import triggerInductorTestsBot from "../lib/bot/triggerInductorTestsBot";
5+
import * as utils from "./utils";
6+
7+
nock.disableNetConnect();
8+
9+
describe("utils: hasApprovedPullRuns", () => {
10+
let probot: Probot;
11+
let octokit = utils.testOctokit();
12+
let REPO = "pytorch/pytorch";
13+
let SHA = "random sha";
14+
15+
beforeEach(() => {
16+
probot = utils.testProbot();
17+
probot.load(triggerInductorTestsBot);
18+
});
19+
20+
function mockRuns(
21+
runs: { conclusion: string; created_at?: string; updated_at?: string }[]
22+
) {
23+
return nock("https://api.github.com")
24+
.get(`/repos/${REPO}/actions/runs?head_sha=${SHA}`)
25+
.reply(200, {
26+
workflow_runs: runs.map((run) => ({
27+
event: "pull_request",
28+
...run,
29+
})),
30+
});
31+
}
32+
33+
async function checkhasApprovedPullRunsReturns(value: boolean) {
34+
expect(await hasApprovedPullRuns(octokit, "pytorch", "pytorch", SHA)).toBe(
35+
value
36+
);
37+
}
38+
39+
afterEach(() => {
40+
nock.cleanAll();
41+
jest.restoreAllMocks();
42+
});
43+
44+
test("successful runs = good", async () => {
45+
mockRuns([{ conclusion: "success" }, { conclusion: "success" }]);
46+
await checkhasApprovedPullRunsReturns(true);
47+
});
48+
49+
test("at least 1 action required run = bad", async () => {
50+
mockRuns([{ conclusion: "action_required" }, { conclusion: "success" }]);
51+
await checkhasApprovedPullRunsReturns(false);
52+
});
53+
54+
test("no runs = bad", async () => {
55+
mockRuns([]);
56+
await checkhasApprovedPullRunsReturns(false);
57+
});
58+
59+
test("one startup failure = bad", async () => {
60+
mockRuns([
61+
{
62+
conclusion: "failure",
63+
created_at: "time",
64+
updated_at: "time",
65+
},
66+
]);
67+
await checkhasApprovedPullRunsReturns(false);
68+
});
69+
70+
test("one startup failure and one action required = bad", async () => {
71+
mockRuns([
72+
{
73+
conclusion: "failure",
74+
created_at: "time",
75+
updated_at: "time",
76+
},
77+
{ conclusion: "action_required" },
78+
]);
79+
await checkhasApprovedPullRunsReturns(false);
80+
});
81+
82+
test("one startup failure and one success = bad", async () => {
83+
mockRuns([
84+
{
85+
conclusion: "failure",
86+
created_at: "time",
87+
updated_at: "time",
88+
},
89+
{ conclusion: "success" },
90+
]);
91+
await checkhasApprovedPullRunsReturns(false);
92+
});
93+
});

0 commit comments

Comments
 (0)