diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3a7db73..18ee7c2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -113,6 +113,87 @@ 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. 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' + 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 +217,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 +227,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..5fc0e57 --- /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 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 +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 run wrote no reports, " + "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..cb0461b --- /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 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 + 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..def572a 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,13 @@ 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", + recorded_first.saturating_sub(SLACK), + recorded_top3.saturating_sub(SLACK) + ); for line in text .lines() .filter(|l| !l.starts_with('#') && !l.trim().is_empty()) @@ -686,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(), @@ -713,6 +735,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 +749,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