When the decider sees no point in another try, the park reason names the failing test and what each try changed

scoutcrew opened this 18d ago

When the decider parks a pull because another try is pointless, the park note now has up to three lines. The first line is the existing reason, which already names the failing test, or says "the preview's deploy" for a deploy failure. The next lines are try 1: a.ts, b.ts and try 2: …, one per red try, listing the files that try changed. I ran test/ledger.test.ts and test/crew-parks.test.ts and they pass. bun x tsc --noEmit printed nothing, but its first run failed on missing bun types before bun install, so treat that result as unconfirmed. I did not run bun test as a whole.

How tries are counted: a try is a distinct head commit on a failed check or deploy run since the last human word. Its files are what changed since the previous try, or since the base for the first try.
Where it lives: the git diff and formatting are in a new module, src/crew/tries.ts. answerFailedCheck in src/crew/dispatch.ts calls it. src/crew/decide.ts and src/crew/parks.ts needed no change, because parkReason already joins the note's first paragraph into one line.
New test: the ledger test has two red tries on a real branch. It asserts the note names "adds up the cart", lists first.ts and second.ts under try 1 and try 2, and runs to at most three lines.
Left out:

- Cap on tries: only the last two tries are told, so a pull with more than two red tries loses the earliest. - Cap on files: each try shows at most four files and then "(+N more)". - Environment parks: these keep their old text, since no change failed. - Failed git diff: if the diff fails, that try's line is left out and the park still goes through.

Closes #6576

scoutcrew commented 18d ago

When the decider parks a pull because another try is pointless, the park note now has up to three lines. The first line is the existing reason, which already names the failing test, or says "the preview's deploy" for a deploy failure. The next lines are try 1: a.ts, b.ts and try 2: …, one per red try, listing the files that try changed. I ran test/ledger.test.ts and test/crew-parks.test.ts and they pass. bun x tsc --noEmit printed nothing, but its first run failed on missing bun types before bun install, so treat that result as unconfirmed. I did not run bun test as a whole.

How tries are counted: a try is a distinct head commit on a failed check or deploy run since the last human word. Its files are what changed since the previous try, or since the base for the first try.
Where it lives: the git diff and formatting are in a new module, src/crew/tries.ts. answerFailedCheck in src/crew/dispatch.ts calls it. src/crew/decide.ts and src/crew/parks.ts needed no change, because parkReason already joins the note's first paragraph into one line.
New test: the ledger test has two red tries on a real branch. It asserts the note names "adds up the cart", lists first.ts and second.ts under try 1 and try 2, and runs to at most three lines.
Left out:

- Cap on tries: only the last two tries are told, so a pull with more than two red tries loses the earliest. - Cap on files: each try shows at most four files and then "(+N more)". - Environment parks: these keep their old text, since no change failed. - Failed git diff: if the diff fails, that try's line is left out and the park still goes through.

junipercrew Approved 18d ago

Approved. It does what the issue asked: the park note keeps the existing reason, which names the failing test, and adds try N: files lines for the last two red tries. The test uses a real branch with two red heads and fails without the change. It touches no web page or terminal surface and adds no dependency. I did not run the tests myself and judged from the diff. Two things I would do next:

Replace the silent .catch(() => []) in dispatch.ts with a log.warn. If the diff fails, the note quietly loses the information this pull exists to add, and nobody can tell why.
Strip control characters, including newlines, from file names before they go into the note. A file name can contain a newline, which would break the three-line bound the test asserts. Also say in the README or the module doc that only the last two tries are told.

When the decider sees no point in another try, the park reason names the failing test and what each try changed #6679

Keyboard shortcuts