From bd4c759a35034d60836c26456b88ecf2776bacb1 Mon Sep 17 00:00:00 2001 From: Marcus Kainth Date: Fri, 25 Sep 2026 11:35:12 +0100 Subject: [PATCH] ci: check that every test runs in exactly one group A group's filter that stops matching some tests leaves them running nowhere, and CI stays green. group_coverage.rs caught that for the simulation suites only, by reading the script's lettered filters. Every other package's tests, and the split of the driver's native_* suites between native-rest and emulator, had no check. scripts/test-group.sh gains two modes. `--list ` prints the tests the group selects, through `cargo nextest list` with the same archives and filters the run uses. `--check` lists every test in the archives (or the workspace, without NEXTEST_ARCHIVE_DIR) and every group, and fails when a test is selected by no group or by two, or when a group selects nothing. A test a group reaches from two archives counts once for that group: four refemu tests in probe_fixture and reference_trace run in both the debug workspace archive and the release ROM suites archive, both in emulator. The new test-groups job runs --check against the three archives, and ci-passed needs it. group_coverage.rs also checks that ci.yml's test matrix, the script's case arms and its new `groups=(...)` line name the same groups, and it reads group names of any length rather than one letter. --- .github/workflows/ci.yml | 29 ++++++- DEVELOPING.md | 5 +- native/tests/group_coverage.rs | 140 ++++++++++++++++++++++++--------- scripts/test-group.sh | 72 +++++++++++++++-- 4 files changed, 202 insertions(+), 44 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5043b8e..4a2971a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -246,6 +246,33 @@ jobs: chmod +x target/release/refemu scripts/test-group.sh "$GROUP" + test-groups: + # Every test in the archives is selected by exactly one group of the + # test matrix, and every group selects at least one test. Lists the + # tests and runs none, so it needs no ClickHouse. + needs: [build-tests, build-refemu] + runs-on: ubuntu-latest + permissions: + contents: read + timeout-minutes: 10 + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + - uses: taiki-e/install-action@3f74d7c16a4242f1c95561e98edc25d36adb4375 # v2.87.12 + with: + tool: cargo-nextest@0.9.140 + - name: Download the archives + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + with: + pattern: "*-{tests,suites}" + merge-multiple: true + path: target/nextest + - name: Every test in exactly one group + env: + NEXTEST_ARCHIVE_DIR: target/nextest + run: scripts/test-group.sh --check + native-smoke: # The driver's own path to a frame: load the schema and the committed # probe fixture, render the frame the fixture's metadata names, compare @@ -322,7 +349,7 @@ jobs: # result, which is a failure if any group failed. A job added to this # workflow goes in `needs`, or a branch rule that requires this check does # not wait for it. - needs: [lint, build-rom, build-refemu, build-tests, test, native-smoke, differential-smoke] + needs: [lint, build-rom, build-refemu, build-tests, test, test-groups, native-smoke, differential-smoke] if: always() runs-on: ubuntu-latest permissions: {} diff --git a/DEVELOPING.md b/DEVELOPING.md index 55b3176..565c4f6 100644 --- a/DEVELOPING.md +++ b/DEVELOPING.md @@ -52,7 +52,10 @@ What `make help` does not say: lists the groups. One of them takes every simulation suite the others do not name, so a suite added to the tree runs there until somebody packs it; `native/tests/group_coverage.rs` fails if that group stops existing, - or if two groups name the same suite. `make test-group + if two groups name the same suite, or if the CI matrix and the script + name different groups. `scripts/test-group.sh --check`, which CI runs + against the archives, fails if any test in any package is in no group or + in two, or if a group selects nothing. `make test-group GROUP=native-sim-a` runs one group the way CI does, building what it needs (`cargo install cargo-nextest --locked` first); `make test` runs every suite in one pass. Two repository diff --git a/native/tests/group_coverage.rs b/native/tests/group_coverage.rs index 6f04dba..519069b 100644 --- a/native/tests/group_coverage.rs +++ b/native/tests/group_coverage.rs @@ -1,13 +1,15 @@ -//! Every simulation suite is selected by exactly one CI group. +//! Every simulation suite is selected by exactly one CI group, and CI runs +//! every group the script defines. //! -//! `scripts/test-group.sh` splits the simulation suites across the lettered -//! groups by name, and one group takes everything the others do not name. -//! A suite named by no group would run nowhere and CI would stay green -//! without it; a suite named by two would run twice and cost a group its -//! budget. Neither shows up in a run's own output, so this reads the -//! script's own filters and the suites on disk and says which. +//! `scripts/test-group.sh` splits the simulation suites across the named +//! groups, and one group takes everything the others do not name. A suite +//! named by no group would run nowhere and CI would stay green without it; +//! a suite named by two would run twice and cost a group its budget. +//! Neither shows up in a run's own output, so this reads the script's own +//! filters and the suites on disk and says which. `test-group.sh --check` +//! makes the same check over the built archives, for every package. //! -//! This parses the `binary(...)` names out of the `sim_=` lines +//! This parses the `binary(...)` names out of the `sim_=` lines //! rather than evaluating a filterset, so a filter that stops being a //! plain list of `binary()` terms fails here rather than being read wrong. @@ -37,18 +39,18 @@ fn suites_on_disk() -> Vec { found } -/// The suites each `sim_=` line names, by letter. -fn named_by_letter(script: &str) -> BTreeMap> { +/// The suites each `sim_=` line names, by name. +fn named_by_group(script: &str) -> BTreeMap> { let mut named = BTreeMap::new(); for line in script.lines() { - let Some(rest) = line.strip_prefix("sim_") else { + let Some((name, body)) = line + .strip_prefix("sim_") + .and_then(|rest| rest.split_once('=')) + .filter(|(name, _)| name.chars().all(|c| c.is_ascii_alphanumeric())) + else { continue; }; - let mut chars = rest.chars(); - let (Some(letter), Some('=')) = (chars.next(), chars.next()) else { - continue; - }; - let body = rest[2..].trim_matches('\''); + let body = body.trim_matches('\''); let mut suites = Vec::new(); for term in body.split('|') { let term = term.trim(); @@ -56,32 +58,47 @@ fn named_by_letter(script: &str) -> BTreeMap> { .strip_prefix("binary(") .and_then(|t| t.strip_suffix(')')) .unwrap_or_else(|| { - panic!("sim_{letter} holds {term}, which is not a plain binary() term") + panic!("sim_{name} holds {term}, which is not a plain binary() term") }); suites.push(inner.to_owned()); } - named.insert(letter, suites); + named.insert(name.to_owned(), suites); } - assert!(!named.is_empty(), "the script names no lettered group"); + assert!(!named.is_empty(), "the script names no simulation group"); named } -/// The letter whose case runs everything the others do not name. -fn catch_all(script: &str) -> char { - let mut found: Option = None; - let mut case: Option = None; +/// The label of each `case` arm that names a group, in order. Arms for +/// options (`--check`) and the fallback (`*`) are left out. +fn case_labels(script: &str) -> Vec { + script + .lines() + .filter_map(|line| { + let label = line.strip_prefix(" ")?.strip_suffix(')')?; + (label.starts_with(|c: char| c.is_ascii_alphanumeric()) + && label.chars().all(|c| c.is_ascii_alphanumeric() || c == '-')) + .then(|| label.to_owned()) + }) + .collect() +} + +/// The name whose case runs everything the others do not name. +fn catch_all(script: &str) -> String { + let mut found: Option = None; + let mut case: Option = None; for line in script.lines() { - if let Some(rest) = line - .trim() - .strip_prefix("native-sim-") - .filter(|rest| rest.trim_end().ends_with(')')) + if let Some(name) = line + .strip_prefix(" native-sim-") + .and_then(|rest| rest.strip_suffix(')')) { - case = rest.chars().next(); + case = Some(name.to_owned()); } if line.contains("binary(/^sim_/) and not (") { - let letter = case.expect("the catch-all filter sits inside a native-sim- case"); + let name = case + .clone() + .expect("the catch-all filter sits inside a native-sim- case"); assert!(found.is_none(), "two groups claim to be the catch-all"); - found = Some(letter); + found = Some(name); } } found.expect( @@ -93,23 +110,23 @@ fn catch_all(script: &str) -> char { fn every_simulation_suite_runs_in_exactly_one_group() { let script = std::fs::read_to_string(repo_root().join("scripts/test-group.sh")) .expect("the script reads"); - let named = named_by_letter(&script); + let named = named_by_group(&script); let rest = catch_all(&script); assert!( !named.contains_key(&rest), "native-sim-{rest} both names suites and takes the rest", ); - let mut groups: BTreeMap> = BTreeMap::new(); + let mut groups: BTreeMap> = BTreeMap::new(); for suite in suites_on_disk() { groups.entry(suite).or_default(); } - for (letter, suites) in &named { + for (name, suites) in &named { for suite in suites { groups .get_mut(suite) - .unwrap_or_else(|| panic!("sim_{letter} names {suite}, which is not on disk")) - .push(*letter); + .unwrap_or_else(|| panic!("sim_{name} names {suite}, which is not on disk")) + .push(name.clone()); } } @@ -133,3 +150,56 @@ fn every_simulation_suite_runs_in_exactly_one_group() { "every suite is named, so native-sim-{rest} runs nothing and its budget is wasted" ); } + +/// The `groups=(...)` list, which `--check` walks and the usage line prints. +fn listed_groups(script: &str) -> Vec { + let line = script + .lines() + .find_map(|line| line.strip_prefix("groups=(")) + .and_then(|rest| rest.strip_suffix(')')) + .expect("the script lists its groups on one `groups=(...)` line"); + line.split_whitespace().map(str::to_owned).collect() +} + +/// The `group: [...]` list of the test matrix in ci.yml. +fn matrix_groups(workflow: &str) -> Vec { + let line = workflow + .lines() + .find_map(|line| line.trim().strip_prefix("group: [")) + .and_then(|rest| rest.strip_suffix(']')) + .expect("ci.yml's test matrix lists its groups on one `group: [...]` line"); + line.split(',').map(|g| g.trim().to_owned()).collect() +} + +#[test] +fn ci_runs_every_group_the_script_defines() { + let script = std::fs::read_to_string(repo_root().join("scripts/test-group.sh")) + .expect("the script reads"); + let workflow = std::fs::read_to_string(repo_root().join(".github/workflows/ci.yml")) + .expect("the workflow reads"); + + let mut listed = listed_groups(&script); + let mut cases = case_labels(&script); + let mut matrix = matrix_groups(&workflow); + assert!( + listed.len() > 1, + "the groups line was read as {listed:?}, which cannot be right" + ); + for names in [&mut listed, &mut cases, &mut matrix] { + names.sort(); + } + assert_eq!( + cases, listed, + "the script's case arms and its groups line name different groups" + ); + assert_eq!( + matrix, listed, + "ci.yml's test matrix and the script's groups line name different groups" + ); + for (name, _) in named_by_group(&script) { + assert!( + listed.contains(&format!("native-sim-{name}")), + "sim_{name} names suites for native-sim-{name}, which is not a group" + ); + } +} diff --git a/scripts/test-group.sh b/scripts/test-group.sh index b6079db..e94bfec 100755 --- a/scripts/test-group.sh +++ b/scripts/test-group.sh @@ -3,6 +3,13 @@ # Runs one group of the test suites, the way ci.yml runs them in parallel. # # scripts/test-group.sh +# scripts/test-group.sh --list +# scripts/test-group.sh --check +# +# --list prints the tests a group selects, one ` ` per +# line, and runs nothing. --check lists every group and every test the +# suites hold, and fails when a test is in no group or in two, or when a +# group selects nothing. # # Groups, and what they hold: # emulator every suite outside the native crate and the driver's @@ -35,8 +42,32 @@ set -euo pipefail cd "$(dirname "$0")/.." +groups=(emulator native-sim-a native-sim-b native-sim-c native-sim-d native-sim-e native-sim-f native-rest) + +verb=run +if [ "${1-}" = --list ]; then + verb=list + shift +fi group="${1-}" +# `cargo nextest run` with the arguments given, or under --list +# `cargo nextest list` with the same selection. +nextest() { + if [ "$verb" = run ]; then + cargo nextest run "$@" + return + fi + local selection=() + while [ $# -gt 0 ]; do + case "$1" in + --test-threads) shift 2 ;; + *) selection+=("$1"); shift ;; + esac + done + cargo nextest list --message-format oneline "${selection[@]}" +} + # With NEXTEST_ARCHIVE_DIR set, the suites come pre-built from # `cargo nextest archive` and nothing is compiled here: native.tar.zst holds # the native crate's suites, workspace.tar.zst every other crate's with the @@ -50,19 +81,19 @@ if [ -n "$archive" ]; then # at build time, which is the workspace's own target directory. extract=(--workspace-remap . --extract-to . --extract-overwrite) run_native() { - cargo nextest run --archive-file "$archive/native.tar.zst" "${extract[@]}" "$@" + nextest --archive-file "$archive/native.tar.zst" "${extract[@]}" "$@" } run_workspace() { - cargo nextest run --archive-file "$archive/workspace.tar.zst" "${extract[@]}" "$@" + nextest --archive-file "$archive/workspace.tar.zst" "${extract[@]}" "$@" } run_rom() { - cargo nextest run --archive-file "$archive/rom-suites.tar.zst" "${extract[@]}" "$@" + nextest --archive-file "$archive/rom-suites.tar.zst" "${extract[@]}" "$@" } live="" else - run_native() { cargo nextest run --locked "$@"; } - run_workspace() { cargo nextest run --locked "$@"; } - run_rom() { cargo nextest run --locked --release -p refemu --features rom-tests "$@"; } + run_native() { nextest --locked "$@"; } + run_workspace() { nextest --locked "$@"; } + run_rom() { nextest --locked --release -p refemu --features rom-tests "$@"; } live="--workspace --features clickhouse-tests" fi @@ -89,6 +120,33 @@ sim_d='binary(sim_floor_live) | binary(sim_hearing_live) | binary(sim_justattack sim_f='binary(sim_aim_live) | binary(sim_damage_live) | binary(sim_impact_live) | binary(sim_noise_live) | binary(sim_parity_live) | binary(sim_plat_live) | binary(sim_removed_live) | binary(sim_sight_live)' case "$group" in + --check) + listed=$(mktemp -d) + trap 'rm -rf "$listed"' EXIT + verb=list + # shellcheck disable=SC2086 + { run_native $live; run_workspace $live; run_rom; } | sort -u > "$listed/all" + failed=0 + for g in "${groups[@]}"; do + "$0" --list "$g" | sort -u | sed "s|^|$g |" > "$listed/group-$g" + echo "$g: $(wc -l < "$listed/group-$g") tests" + if [ ! -s "$listed/group-$g" ]; then + echo "::error::$g selects no tests" >&2 + failed=1 + fi + done + cat "$listed"/group-* | cut -d' ' -f2- | sort > "$listed/selected" + echo "every test: $(wc -l < "$listed/all"); selected: $(wc -l < "$listed/selected")" + if uniq -d "$listed/selected" | grep .; then + echo "::error::the tests above are selected by more than one group" >&2 + failed=1 + fi + if comm -23 "$listed/all" <(sort -u "$listed/selected") | grep .; then + echo "::error::the tests above are selected by no group" >&2 + failed=1 + fi + exit "$failed" + ;; emulator) # One test at a time: the SQL CPU's suite and the executor's share # the server's compiled-expression cache, which a second run beside @@ -147,7 +205,7 @@ case "$group" in -E 'package(clickdoom-driver) and binary(/^native_/) and not binary(native_connections_live)' ;; *) - echo "usage: scripts/test-group.sh emulator|native-sim-a|native-sim-b|native-sim-c|native-sim-d|native-sim-e|native-sim-f|native-rest" >&2 + echo "usage: scripts/test-group.sh [--list] $(IFS='|'; echo "${groups[*]}") | --check" >&2 exit 2 ;; esac