diff --git a/actions/experiment/README.md b/actions/experiment/README.md index 1a6e3bd0..dd654d87 100644 --- a/actions/experiment/README.md +++ b/actions/experiment/README.md @@ -5,7 +5,7 @@ This composite action runs your eval script in a GitHub Actions workflow. The sc result to the job summary and to a comment on the pull request. The job fails when the check of the pass marks of an experiment fails, when an experiment does not complete, or when the script exits with an error. A script that catches the error of one experiment and runs the next still fails the -job. +job, and so does a failed experiment that runs with `ci: false` (`ci=False` in Python). ```yaml on: pull_request diff --git a/actions/experiment/report.mjs b/actions/experiment/report.mjs index dc35834f..6e9f13aa 100644 --- a/actions/experiment/report.mjs +++ b/actions/experiment/report.mjs @@ -6,8 +6,8 @@ import { appendFileSync, readFileSync } from "node:fs"; const RATE_KINDS = new Set(["pass_fail", "up_down"]); /** - * Pages of 100 comments that the action reads to find its comment from an earlier push: the first - * page, then the newest pages. + * Pages of 100 comments that the action reads to find its comment from an earlier push: the oldest + * half, then the newest half. */ const MAX_COMMENT_PAGES = 10; /** The longest comment body that GitHub takes, in characters. */ @@ -279,13 +279,19 @@ async function comment(report) { const first = await list(1); let existing = find(first); - // The list starts with the oldest comments. On a long pull request the comment can be past the - // pages that the action reads, and then the comment that it adds is on the newest page. - for ( - let page = lastPage(first.link), read = 1; - existing === undefined && page > 1 && read < MAX_COMMENT_PAGES; - page--, read++ - ) { + // The list starts with the oldest comments, and a comment stays on its page. The comment of the + // first push is on an old page. On a long pull request it can be past the oldest pages, and then + // the comment that the action adds is on a new page. + const half = MAX_COMMENT_PAGES / 2; + const last = lastPage(first.link); + const pages = []; + + for (let page = 2; page <= Math.min(half, last); page++) pages.push(page); + for (let page = last; page > Math.max(half, last - half); page--) pages.push(page); + + for (const page of pages) { + if (existing !== undefined) break; + existing = find(await list(page)); } diff --git a/packages/sdk/README.md b/packages/sdk/README.md index dbceb505..25bd67af 100644 --- a/packages/sdk/README.md +++ b/packages/sdk/README.md @@ -319,7 +319,8 @@ the result on the pull request. It gives runExperiment the `dataset` input in `TELEMETRY_DEV_EXPERIMENT_DATASET`, which must agree with the `dataset` of the script, and the `min-scores` input in `TELEMETRY_DEV_EXPERIMENT_MIN_SCORES`, which wins over `minScores` for the same name. Use the action as `telemetry-dev/telemetry/actions/experiment@experiment-action-v1`, a -tag that moves to each 1.x release of the action. +tag that moves to each 1.x release of the action. In the action, a failed experiment fails the job +also with `ci: false`. ## Scores diff --git a/packages/sdk/tests/experiment-action.test.ts b/packages/sdk/tests/experiment-action.test.ts index 89f49bb7..8d75f142 100644 --- a/packages/sdk/tests/experiment-action.test.ts +++ b/packages/sdk/tests/experiment-action.test.ts @@ -469,20 +469,55 @@ test("report.mjs gives each matrix leg and working directory its own comment, wh expect(changes(again)).toEqual(["PATCH /repos/acme/support/issues/comments/8"]); }); -test("report.mjs finds its comment on the newest page of a pull request with more than 1,000 comments", async () => { - const own = { id: 1006, body: await commentBody(), user: BOT }; - const others = (from: number, count: number) => - Array.from({ length: count }, (_, index) => ({ - id: from + index, - body: "Looks good", - user: { login: "dana", type: "User" as const }, - })); - const github = await fakeGithub({ comments: [...others(1, 1005), own, ...others(1007, 10)] }); - - await report({ GITHUB_API_URL: github.url }, [result]); - - expect(changes(github)).toEqual(["PATCH /repos/acme/support/issues/comments/1006"]); -}); +test.each([ + ["on the newest page", 1005, 10, [1, 2, 3, 4, 5, 11]], + ["on an old page", 149, 1000, [1, 2]], + ["on the last old page that it reads", 449, 700, [1, 2, 3, 4, 5]], +])( + "report.mjs finds its comment %s of a pull request with more than 1,000 comments", + async (_, before, after, pages) => { + const own = { id: before + 1, body: await commentBody(), user: BOT }; + const github = await fakeGithub({ + comments: [...others(1, before), own, ...others(before + 2, after)], + }); + + await report({ GITHUB_API_URL: github.url }, [result]); + + expect(listedPages(github)).toEqual(pages); + expect(changes(github)).toEqual([`PATCH /repos/acme/support/issues/comments/${before + 1}`]); + }, +); + +test.each([ + ["12 pages", 1150, [1, 2, 3, 4, 5, 12, 11, 10, 9, 8]], + ["7 pages", 650, [1, 2, 3, 4, 5, 7, 6]], +])( + "report.mjs reads each page once on a pull request of %s without its comment, and adds the comment", + async (_, count, pages) => { + const github = await fakeGithub({ comments: others(1, count) }); + + await report({ GITHUB_API_URL: github.url }, [result]); + + expect(listedPages(github)).toEqual(pages); + expect(changes(github)).toEqual(["POST /repos/acme/support/issues/42/comments"]); + }, +); + +/** Comments by a person, with the ids from `from`. */ +function others(from: number, count: number) { + return Array.from({ length: count }, (_, index) => ({ + id: from + index, + body: "Looks good", + user: { login: "dana", type: "User" as const }, + })); +} + +/** The pages of comments that the action read, in order. */ +function listedPages(github: Awaited>): number[] { + return github.requests + .filter((request) => request.method === "GET" && request.url.includes("/comments?")) + .map((request) => Number(new URL(request.url, "http://github.test").searchParams.get("page"))); +} test.each([ ["a person pasted under the GITHUB_TOKEN", undefined, { login: "mallory", type: "User" }], diff --git a/sdks/python/README.md b/sdks/python/README.md index bef0cf95..08ae3785 100644 --- a/sdks/python/README.md +++ b/sdks/python/README.md @@ -411,7 +411,8 @@ the result on the pull request. It gives `run_experiment` the `dataset` input in `TELEMETRY_DEV_EXPERIMENT_DATASET`, which must agree with the `dataset` of the script, and the `min-scores` input in `TELEMETRY_DEV_EXPERIMENT_MIN_SCORES`, which wins over `min_scores` for the same name. Use the action as `telemetry-dev/telemetry/actions/experiment@experiment-action-v1`, a -tag that moves to each 1.x release of the action. +tag that moves to each 1.x release of the action. In the action, a failed experiment fails the job +also with `ci=False`. ## Scores