fix(sdk): find the comment of the action on the old pages of a long pull request

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

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));
   }
 

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
 

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<ReturnType<typeof fakeGithub>>): 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" }],

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