From 79368efd7f92c89c98fa1ebc9459d43e9589f0f3 Mon Sep 17 00:00:00 2001 From: Mofei Zhu Date: Wed, 21 Oct 2026 11:05:50 +0300 Subject: [PATCH 1/2] Report commands search eval scores against the base in CI A search-eval job runs the eval sets on the PR and its base and writes the scores, and the queries the change fixed and broke, to the job summary. It never fails: the eval_set tests are the gate. --- .github/workflows/ci.yml | 89 ++++++- scripts/commands-search-report.py | 291 +++++++++++++++++++++++ scripts/test-commands-search-report.sh | 167 +++++++++++++ src/commands.rs | 31 +++ tests/fixtures/commands_search/README.md | 7 +- 5 files changed, 583 insertions(+), 2 deletions(-) create mode 100644 scripts/commands-search-report.py create mode 100755 scripts/test-commands-search-report.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3a7db73..ec5df74 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -113,6 +113,84 @@ jobs: shell: bash run: sh scripts/test-completion.sh + # Which `commands search` queries a PR fixed and broke, against its base, + # written to the job summary. It reports and never fails: the floors that + # `cargo test` holds in the build job are the gate. A job summary rather + # than a PR comment because it needs no write token, so a PR from a fork + # gets one too. One runner: ranking is the same everywhere. + search-eval: + name: commands search eval + runs-on: ubuntu-latest + timeout-minutes: 20 + permissions: + contents: read + steps: + # Whole history, so the base commit is there to check out beside it. + # No credential left behind: nothing here pushes or fetches again. + - uses: actions/checkout@v7 + with: + fetch-depth: 0 + persist-credentials: false + + # As in the build job: install exactly the pinned toolchain. + - name: Read the pinned toolchain + id: toolchain + run: | + echo "toolchain=$(sed -n 's/^channel = "\(.*\)"/\1/p' rust-toolchain.toml | head -1)" >>"$GITHUB_OUTPUT" + + - name: Install Rust + uses: dtolnay/rust-toolchain@stable + with: + toolchain: ${{ steps.toolchain.outputs.toolchain }} + + # The exit status is recorded rather than failing the step: a run below + # its floor still writes its reports, the build job is where that + # failure is reported, and the summary needs the status to say whether + # missing reports mean a failed run or one with no eval to run. + - name: Run the eval on this branch + id: head + run: | + status=0 + MAPBOX_SEARCH_EVAL_REPORT="$RUNNER_TEMP/eval-head" cargo test --locked --bin mapbox eval_set || status=$? + echo "status=$status" >>"$GITHUB_OUTPUT" + + # In a worktree sharing this target directory, so only the crate itself + # rebuilds. A base from before the eval existed matches no test, passes + # and writes no reports; one that fails to build or check out exits + # non-zero. The summary tells the two apart by the status. + - name: Run the eval on the base + id: base + if: github.event_name == 'pull_request' + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + run: | + status=0 + { + git worktree add --detach "$RUNNER_TEMP/base" "$BASE_SHA" && + cd "$RUNNER_TEMP/base" && + CARGO_TARGET_DIR="$GITHUB_WORKSPACE/target" \ + MAPBOX_SEARCH_EVAL_REPORT="$RUNNER_TEMP/eval-base" \ + cargo test --locked --bin mapbox eval_set + } || status=$? + echo "status=$status" >>"$GITHUB_OUTPUT" + + # No `--base` on a push: the script then says there was no base rather + # than that the base had no reports. A status left empty by a step that + # never finished reads as "did not finish". + - name: Write the summary + if: ${{ !cancelled() }} + env: + EVENT: ${{ github.event_name }} + HEAD_STATUS: ${{ steps.head.outputs.status }} + BASE_STATUS: ${{ steps.base.outputs.status }} + run: | + set -- --head "$RUNNER_TEMP/eval-head" --head-status "$HEAD_STATUS" \ + --sets tests/fixtures/commands_search + if [ "$EVENT" = pull_request ]; then + set -- "$@" --base "$RUNNER_TEMP/eval-base" --base-status "$BASE_STATUS" + fi + python3 scripts/commands-search-report.py "$@" >>"$GITHUB_STEP_SUMMARY" + # No Rust: this is where the repo's shell gets checked, install.sh above all # — it is served to machines we do not control, and this is the only thing # that checks it. Both runners, because the portability it has to get right @@ -136,6 +214,7 @@ jobs: sh -n scripts/install.sh sh -n scripts/test-install.sh sh -n scripts/test-completion.sh + sh -n scripts/test-commands-search-report.sh # -s sh, not the default: bash would accept things `curl | sh` will not. # Once is enough — the findings do not depend on the runner. The version @@ -145,11 +224,19 @@ jobs: # Kept clean against 0.9, 0.10 and 0.11. - name: Shellcheck if: runner.os == 'Linux' - run: shellcheck -s sh scripts/install.sh scripts/test-install.sh scripts/test-completion.sh + run: shellcheck -s sh scripts/install.sh scripts/test-install.sh scripts/test-completion.sh scripts/test-commands-search-report.sh - name: Install, verify and prompt against a local channel run: sh scripts/test-install.sh + # The `commands search eval` job's summary script, against hand-written + # reports: that job never fails, so this is where a summary that says + # the wrong thing would be caught. Python only, so it lives here rather + # than beside the Rust. Once is enough, as for shellcheck. + - name: Summarize hand-written search eval reports + if: runner.os == 'Linux' + run: sh scripts/test-commands-search-report.sh + # The other half of the same job, for the installer install.sh stops short # of. Both shells, because both are on a stock Windows 11 and they differ # exactly where this script lives: the default TLS versions, what `2>` does diff --git a/scripts/commands-search-report.py b/scripts/commands-search-report.py new file mode 100644 index 0000000..7870743 --- /dev/null +++ b/scripts/commands-search-report.py @@ -0,0 +1,291 @@ +#!/usr/bin/env python3 +"""Markdown summary of `mapbox commands search` eval results, for a CI job +summary. + + commands-search-report.py --head DIR --head-status N + [--base DIR --base-status N] [--sets DIR] + +Each directory holds the per-set reports the `eval_set_*` tests write when +MAPBOX_SEARCH_EVAL_REPORT names it; each status is the exit code of the +`cargo test` run that wrote them. No `--base` means there was no base to run +(not a pull request). `--sets` is the fixture directory, so a set whose +report is missing is named rather than left out. + +The status is what tells apart a base that predates the eval (the run passed +and wrote nothing) from one whose run failed: the reports alone look the +same for both. + +Reports, never gates: the floors in `cargo test` are the gate. So this never +fails the job on bad input: a malformed line is skipped and counted, and an +unreadable report is named. Standard library only, so the job needs no +install step. +""" + +import argparse +import sys +from pathlib import Path + + +class Set: + def __init__(self): + self.floors = None + self.rows = {} + self.invalid = [] + self.skipped = 0 + self.unreadable = None + + +def load(directory): + """Every report in `directory`, by set name. Empty when there is none.""" + sets = {} + if directory is None or not directory.is_dir(): + return sets + for path in sorted(directory.iterdir()): + report = sets[path.name] = Set() + try: + text = path.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError) as error: + report.unreadable = str(error) + continue + for line in text.splitlines(): + parse_line(report, line) + return sets + + +def parse_line(report, line): + columns = line.split("\t") + if columns[0] == "# floors": + try: + report.floors = (int(columns[1]), int(columns[2])) + except (IndexError, ValueError): + report.skipped += 1 + return + if columns[0] == "# invalid": + report.invalid.append(" ".join(columns[1:])) + return + if columns[0].startswith("#") or not line.strip(): + return + if len(columns) != 5 or columns[3] not in ("0", "1") or columns[4] not in ("0", "1"): + report.skipped += 1 + return + query, expected, found, first, top3 = columns + report.rows[query] = { + "expected": " or ".join(expected.split("|")) if expected else "(nothing)", + "found": ", ".join(found.split("|")) if found else "(nothing)", + "first": first == "1", + "top3": top3 == "1", + } + + +def cell(text): + """`text` made safe for one Markdown table cell.""" + return ( + str(text) + .replace("&", "&") + .replace("<", "<") + .replace(">", ">") + .replace("|", "\\|") + .replace("\n", " ") + ) + + +def score(report, key): + return sum(row[key] for row in report.rows.values()) + + +# Under the table, so a reader need not know the tests to read it. +LEGEND = ( + "**Ranked first**: the expected command was the first result. " + "**In top 3**: it was among the first three. " + "**Required**: the score in `EVAL_SETS` (src/commands.rs) less its slack; a lower score fails `cargo test`. " + "A query the CLI cannot serve counts as right only when nothing comes back. " + "How each set was gathered is in tests/fixtures/commands_search/README.md." +) + + +def count(head, total, base): + """`22 (37%)`, with the change against the base when there is one: + `22 (37%, +1)`.""" + share = f"{round(100 * head / total)}%" if total else "-" + change = head - base if base is not None else 0 + return f"{head} ({share}, {change:+d})" if change else f"{head} ({share})" + + +def changed(head_rows, base_rows): + """Queries in both whose first result went from right to wrong or back.""" + fixed, broken = [], [] + for query, row in head_rows.items(): + before = base_rows.get(query) + if before is None or before["first"] == row["first"]: + continue + line = ( + f"| {cell(query)} | {cell(row['expected'])} | " + f"{cell(before['found'])} | {cell(row['found'])} |" + ) + (fixed if row["first"] else broken).append(line) + return fixed, broken + + +def status_note(status): + return "did not finish" if status is None else f"failed (exit {status})" + + +def parse_status(text): + try: + return int(text) + except (TypeError, ValueError): + return None + + +def base_note(base_given, base, base_status): + """Why there is nothing to compare against, or what to know about it.""" + if not base_given: + return "Not a pull request, so there is no base to compare against." + if not base: + if base_status == 0: + return ( + "The base predates the eval (its run had no eval tests), " + "so only this branch's scores are shown." + ) + return ( + f"The base run {status_note(base_status)} before writing " + "any report, so only this branch's scores are shown." + ) + if base_status != 0: + return ( + f"The base run {status_note(base_status)}; " + "its scores are as far as it got." + ) + return None + + +def notes(report, floors_missed): + out = [] + if report.unreadable: + out.append(f"unreadable: {report.unreadable}") + if floors_missed: + out.append("**below its floor**") + if report.invalid: + out.append(f"{len(report.invalid)} invalid rows") + if report.skipped: + out.append(f"{report.skipped} malformed lines skipped") + return out + + +def render(head, head_status, base, base_given, base_status, expected_sets): + out = ["## `commands search` eval", ""] + if not head: + if head_status == 0: + out.append("No reports were written, though the eval run passed: no `eval_set_*` test ran.") + else: + out.append( + f"The eval run on this branch {status_note(head_status)} " + "before writing any report. The build job's test step has the details." + ) + return out + if head_status != 0: + out += [ + f"The eval run on this branch {status_note(head_status)}: a set is " + "below its floor, a row is invalid, or the tests did not build. The build " + "job's test step has the details.", + "", + ] + note = base_note(base_given, base, base_status) + if note: + out += [note, ""] + compare = bool(base) + + out += [ + "| Query set | Queries | Ranked first | In top 3 | Required (first / top 3) | Note |", + "| --- | --- | --- | --- | --- | --- |", + ] + details = [] + names = sorted(set(head) | set(base) | set(expected_sets)) + for name in names: + label = f"`{cell(name)}`" + if name not in head: + where = "only in the base" if name in base else "missing from this run" + out.append(f"| {label} | | | | | {where} |") + continue + report = head[name] + base_report = base.get(name) + first, top3 = score(report, "first"), score(report, "top3") + base_first = score(base_report, "first") if base_report else None + base_top3 = score(base_report, "top3") if base_report else None + floors = report.floors + floor = f"{floors[0]} / {floors[1]}" if floors else "-" + missed = bool(floors) and (first < floors[0] or top3 < floors[1]) + row_notes = notes(report, missed) + if compare and base_report is None: + row_notes.insert(0, "only in this branch") + out.append( + f"| {label} | {len(report.rows)} | {count(first, len(report.rows), base_first)} | " + f"{count(top3, len(report.rows), base_top3)} | {floor} | {cell('; '.join(row_notes))} |" + ) + + if report.invalid: + details += [f"### {label}: rows that could not be scored", ""] + details += [f"- {cell(problem)}" for problem in report.invalid] + [""] + + if base_report: + fixed, broken = changed(report.rows, base_report.rows) + header = ["| Query | Want | Base | This branch |", "| --- | --- | --- | --- |"] + if broken: + details += [f"### {label}: {len(broken)} no longer first", ""] + header + broken + [""] + if fixed: + details += [f"### {label}: {len(fixed)} now first", ""] + header + fixed + [""] + + misses = [ + f"| {cell(query)} | {cell(row['expected'])} | {cell(row['found'])} |" + for query, row in report.rows.items() + if not row["first"] + ] + if misses: + details += [ + f"
{cell(name)}: {len(misses)} not first", + "", + "| Query | Want | Got (top 3) |", + "| --- | --- | --- |", + *misses, + "", + "
", + "", + ] + + return out + ["", LEGEND, "", *details] + + +def main(): + parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + parser.add_argument("--head", type=Path, required=True) + parser.add_argument("--head-status", default="") + parser.add_argument("--base", type=Path) + parser.add_argument("--base-status", default="") + parser.add_argument("--sets", type=Path) + args = parser.parse_args() + + expected_sets = [] + if args.sets is not None and args.sets.is_dir(): + expected_sets = [path.name for path in args.sets.glob("*.tsv")] + lines = render( + load(args.head), + parse_status(args.head_status), + load(args.base), + args.base is not None, + parse_status(args.base_status), + expected_sets, + ) + print("\n".join(lines)) + + +if __name__ == "__main__": + try: + main() + except SystemExit: + raise + except Exception as error: # noqa: BLE001 - a summary, never a gate + # The job is promised not to fail on this script; say what broke in + # the summary itself rather than leaving it empty. + print("## `commands search` eval\n") + print(f"The summary script failed: `{cell(repr(error))}`. This is a bug in the script.") + sys.exit(0) diff --git a/scripts/test-commands-search-report.sh b/scripts/test-commands-search-report.sh new file mode 100755 index 0000000..46b7803 --- /dev/null +++ b/scripts/test-commands-search-report.sh @@ -0,0 +1,167 @@ +#!/bin/sh +# Checks that `scripts/commands-search-report.py` says what happened for each +# way the CI `commands search eval` job can go, from hand-written reports. +# +# The job promises never to fail, and its summary is the only place a +# reviewer sees the result, so a summary that is wrong is worse than none: +# the cases here are the ones it once got wrong or could not tell apart. A +# base whose run failed, a push with no base and a base from before the eval +# all used to read "the base branch has no eval reports"; a malformed line +# raised a traceback and turned the job red. +# +# No Rust and no network: the reports are written here in the format the +# `eval_set_*` tests write. +# +# Usage: +# sh scripts/test-commands-search-report.sh + +set -eu + +SCRIPT="$(dirname "$0")/commands-search-report.py" +PYTHON="${PYTHON:-python3}" + +failures=0 + +say() { + printf '%s\n' "$*" +} + +fail() { + printf 'FAIL: %s\n' "$*" >&2 + failures=$((failures + 1)) +} + +WORK="$(mktemp -d)" +cleanup() { + rm -rf "$WORK" +} +trap cleanup EXIT INT TERM + +# One report: a floors line, then each argument as a row. +report() { + file="$1" + shift + mkdir -p "$(dirname "$file")" + printf '# floors\t1\t1\n' >"$file" + for row in "$@"; do + printf '%s\n' "$row" >>"$file" + done +} + +TAB="$(printf '\t')" +row() { + printf '%s' "$1${TAB}$2${TAB}$3${TAB}$4${TAB}$5" +} + +# Runs the script, which must exit 0, into `$WORK/out`. +summarize() { + if ! "$PYTHON" "$SCRIPT" "$@" >"$WORK/out" 2>"$WORK/err"; then + fail "exited non-zero for: $*: $(cat "$WORK/err")" + return 1 + fi + if [ -s "$WORK/err" ]; then + fail "wrote to stderr for: $*: $(cat "$WORK/err")" + fi +} + +expect() { + case_name="$1" + text="$2" + if grep -qF -- "$text" "$WORK/out"; then + say "ok: $case_name" + else + fail "$case_name: no \"$text\" in:" + sed 's/^/ /' "$WORK/out" >&2 + fi +} + +reject() { + case_name="$1" + text="$2" + if grep -qF -- "$text" "$WORK/out"; then + fail "$case_name: unexpected \"$text\" in:" + sed 's/^/ /' "$WORK/out" >&2 + else + say "ok: $case_name" + fi +} + +SETS="$WORK/sets" +mkdir -p "$SETS" +: >"$SETS/a.tsv" +: >"$SETS/b.tsv" + +HEAD="$WORK/head" +report "$HEAD/a.tsv" \ + "$(row 'upload a font' 'mapbox fonts upload' 'mapbox fonts upload' 1 1)" \ + "$(row 'a | b query' 'mapbox x|mapbox y' 'mapbox z' 0 0)" +report "$HEAD/b.tsv" \ + "$(row 'list styles' 'mapbox styles list' 'mapbox styles list' 1 1)" + +BASE="$WORK/base" +report "$BASE/a.tsv" \ + "$(row 'upload a font' 'mapbox fonts upload' 'mapbox fonts list' 0 1)" \ + "$(row 'a | b query' 'mapbox x|mapbox y' 'mapbox z' 0 0)" + +if summarize --head "$HEAD" --head-status 0 --base "$BASE" --base-status 0 --sets "$SETS"; then + expect "a fix against the base is listed" "1 now first" + expect "a set new in this branch is marked" "only in this branch" + expect "a pipe in a cell is escaped" 'a \| b query' + expect "a score carries its share and its change" "| 1 (50%, +1) |" + expect "an unchanged score carries its share alone" "| 1 (50%) |" + expect "the columns are named" "| Query set | Queries | Ranked first | In top 3 |" + expect "the legend says what the columns mean" "**Ranked first**: the expected command" + reject "no emoji" "⚠" +fi + +if summarize --head "$HEAD" --head-status 0 --sets "$SETS"; then + expect "a push has no base" "Not a pull request" +fi + +if summarize --head "$HEAD" --head-status 0 --base "$WORK/none" --base-status 0 --sets "$SETS"; then + expect "a base from before the eval" "The base predates the eval" +fi + +if summarize --head "$HEAD" --head-status 0 --base "$WORK/none" --base-status 101 --sets "$SETS"; then + expect "a base whose run failed" "The base run failed (exit 101)" +fi + +if summarize --head "$HEAD" --head-status 0 --base "$WORK/none" --base-status '' --sets "$SETS"; then + expect "a base whose step never recorded a status" "The base run did not finish" +fi + +ONLY_A="$WORK/only-a" +report "$ONLY_A/a.tsv" "$(row 'q' 'mapbox x' 'mapbox x' 1 1)" +if summarize --head "$ONLY_A" --head-status 101 --base "$BASE" --base-status 0 --sets "$SETS"; then + expect "a set missing from this run is named" "missing from this run" + expect "a failing head run says so" "failed (exit 101)" +fi + +if summarize --head "$ONLY_A" --head-status 0 --base "$HEAD" --base-status 0; then + expect "a set only in the base is marked" "only in the base" +fi + +if summarize --head "$WORK/none" --head-status 101; then + expect "no reports and a failed run" "before writing any report" +fi + +BAD="$WORK/bad" +report "$BAD/a.tsv" \ + "$(row 'fine' 'mapbox x' 'mapbox x' 1 1)" \ + "too${TAB}few${TAB}columns" \ + "$(row 'odd flags' 'mapbox x' 'mapbox x' yes 1)" +printf '# floors\tnot-a-number\n' >>"$BAD/a.tsv" +printf '# invalid\tmapbox gone is not a command search answers with ("q")\n' >>"$BAD/a.tsv" +printf '\377\376 not utf-8\n' >"$BAD/b.tsv" +if summarize --head "$BAD" --head-status 101 --sets "$SETS"; then + expect "malformed lines are skipped and counted" "3 malformed lines skipped" + expect "an invalid row is listed" "mapbox gone" + expect "an unreadable report is named" "unreadable" +fi + +if [ "$failures" -ne 0 ]; then + say "$failures check(s) failed." + exit 1 +fi + +say "All checks passed." diff --git a/src/commands.rs b/src/commands.rs index 2b3d4a3..fa4eef7 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -657,9 +657,18 @@ mod tests { /// targeted tests below are the hard gate for specific queries. const SLACK: usize = 3; + /// Where [`eval_set_keeps_its_scores`] writes its reports, when set. + const REPORT_DIR_ENV: &str = "MAPBOX_SEARCH_EVAL_REPORT"; + /// Holds one set to its recorded scores, less [`SLACK`]. /// `cargo test eval_set -- --nocapture` prints the scores, and each query /// that did not rank first. One test per set, so they run in parallel. + /// + /// With [`REPORT_DIR_ENV`] set, also writes every query's result to + /// `/`, before anything is asserted, so a failing run still + /// leaves one: a set missing from CI's summary would read as a set + /// nobody added. A row that cannot be scored goes in the report as a + /// `# invalid` line rather than stopping the run there. fn eval_set_keeps_its_scores(wanted: &str) { let &(name, text, recorded_first, recorded_top3) = EVAL_SETS .iter() @@ -677,6 +686,12 @@ mod tests { let (mut rows, mut first, mut top3) = (0, 0, 0); let (mut misses, mut invalid) = (vec![], vec![]); + // The scores below which the test fails, not the recorded ones. + let mut report = format!( + "# floors\t{}\t{}\n", + recorded_first.saturating_sub(SLACK), + recorded_top3.saturating_sub(SLACK) + ); for line in text .lines() .filter(|l| !l.starts_with('#') && !l.trim().is_empty()) @@ -713,6 +728,13 @@ mod tests { found.iter().any(|c| expected.contains(&c.as_str())), ) }; + report.push_str(&format!( + "{query}\t{}\t{}\t{}\t{}\n", + expected.join("|"), + found.join("|"), + u8::from(is_first), + u8::from(in_top3) + )); rows += 1; first += usize::from(is_first); top3 += usize::from(in_top3); @@ -720,6 +742,15 @@ mod tests { misses.push(format!(" \"{query}\": want {expected:?}, got {found:?}")); } } + for problem in &invalid { + report.push_str(&format!("# invalid\t{problem}\n")); + } + if let Some(dir) = std::env::var_os(REPORT_DIR_ENV) { + let dir = std::path::PathBuf::from(dir); + std::fs::create_dir_all(&dir).expect("create the report directory"); + std::fs::write(dir.join(name), report).expect("write the report"); + } + eprintln!( "{name}: {first}/{rows} first (recorded {recorded_first}), \ {top3}/{rows} in top 3 (recorded {recorded_top3})" diff --git a/tests/fixtures/commands_search/README.md b/tests/fixtures/commands_search/README.md index 6ffea84..c5cfdd8 100644 --- a/tests/fixtures/commands_search/README.md +++ b/tests/fixtures/commands_search/README.md @@ -5,7 +5,12 @@ tests in `src/commands.rs` run every file here and fail when a set ranks more than `SLACK` (3) queries fewer right than its recorded score, so a reworded summary that moves one query does not fail the build. `cargo test eval_set -- --nocapture` prints each set's score and every -query that did not rank first. +query that did not rank first. On a PR, CI's `commands search eval` job puts the +scores in its job summary, with the queries the change fixed and broke +against the base branch. It also says when there was nothing to compare +against, and why: a push with no base, a base from before these sets, or a +base run that failed. `scripts/test-commands-search-report.sh` checks the +summary against hand-written reports. ## Format From a5a184db3280d6929d88e242b5b5616bebc0dae7 Mon Sep 17 00:00:00 2001 From: Mofei Zhu Date: Wed, 21 Oct 2026 19:59:33 +0300 Subject: [PATCH 2/2] Address review on the commands search eval summary Say the base run wrote no reports rather than that it predates the eval, since a base with the eval tests but no report writer looks the same. Reject a query repeated in a set, which cargo test counted twice and the summary once. Note why the base worktree must be added after the head build. --- .github/workflows/ci.yml | 9 ++++++--- scripts/commands-search-report.py | 6 +++--- scripts/test-commands-search-report.sh | 2 +- src/commands.rs | 7 +++++++ 4 files changed, 17 insertions(+), 7 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ec5df74..18ee7c2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -155,9 +155,12 @@ jobs: echo "status=$status" >>"$GITHUB_OUTPUT" # In a worktree sharing this target directory, so only the crate itself - # rebuilds. A base from before the eval existed matches no test, passes - # and writes no reports; one that fails to build or check out exits - # non-zero. The summary tells the two apart by the status. + # rebuilds. It rebuilds only because the worktree is added after the + # head build: cargo goes by mtime, and base sources older than that + # build would reuse its binary. Keep this step after the head run. + # A base from before the eval or its reports passes and writes none; + # one that fails to build or check out exits non-zero. The summary + # tells the two apart by the status. - name: Run the eval on the base id: base if: github.event_name == 'pull_request' diff --git a/scripts/commands-search-report.py b/scripts/commands-search-report.py index 7870743..5fc0e57 100644 --- a/scripts/commands-search-report.py +++ b/scripts/commands-search-report.py @@ -11,8 +11,8 @@ (not a pull request). `--sets` is the fixture directory, so a set whose report is missing is named rather than left out. -The status is what tells apart a base that predates the eval (the run passed -and wrote nothing) from one whose run failed: the reports alone look the +The status is what tells apart a base whose run passed and wrote nothing +(one that predates the eval or its reports) from one whose run failed: the reports alone look the same for both. Reports, never gates: the floors in `cargo test` are the gate. So this never @@ -144,7 +144,7 @@ def base_note(base_given, base, base_status): if not base: if base_status == 0: return ( - "The base predates the eval (its run had no eval tests), " + "The base run wrote no reports, " "so only this branch's scores are shown." ) return ( diff --git a/scripts/test-commands-search-report.sh b/scripts/test-commands-search-report.sh index 46b7803..cb0461b 100755 --- a/scripts/test-commands-search-report.sh +++ b/scripts/test-commands-search-report.sh @@ -119,7 +119,7 @@ if summarize --head "$HEAD" --head-status 0 --sets "$SETS"; then fi if summarize --head "$HEAD" --head-status 0 --base "$WORK/none" --base-status 0 --sets "$SETS"; then - expect "a base from before the eval" "The base predates the eval" + expect "a base that wrote no reports" "The base run wrote no reports" fi if summarize --head "$HEAD" --head-status 0 --base "$WORK/none" --base-status 101 --sets "$SETS"; then diff --git a/src/commands.rs b/src/commands.rs index fa4eef7..def572a 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -686,6 +686,7 @@ mod tests { let (mut rows, mut first, mut top3) = (0, 0, 0); let (mut misses, mut invalid) = (vec![], vec![]); + let mut seen = BTreeSet::new(); // The scores below which the test fails, not the recorded ones. let mut report = format!( "# floors\t{}\t{}\n", @@ -701,6 +702,12 @@ mod tests { invalid.push(format!("no expected column: {line:?}")); continue; }; + // Scored twice, it would count twice here but once in CI's + // summary, which keys its rows by query. + if !seen.insert(query) { + invalid.push(format!("\"{query}\" is in the set more than once")); + continue; + } let expected: Vec<&str> = match expected { "-" => vec![], answers => answers.split('|').collect(),