diff --git a/GETTING_STARTED.MD b/GETTING_STARTED.MD index e6fcb61..f1b9cd6 100644 --- a/GETTING_STARTED.MD +++ b/GETTING_STARTED.MD @@ -299,7 +299,10 @@ For a real repository: dependency edges, caching inputs or outputs, aliases, or file capabilities. 4. Start with a narrow command such as `aster test //path/to/project`, then expand to `--all` or `aster affected` after the graph looks correct. -5. Add the verified command to CI. +5. Add the verified command to CI. With `--retry-failed --test-results`, + Aster retries only failing tests and records every result; see + [Test tracking](docs/test-tracking.md) to send them to aster-srv for flake + detection and quarantine. From there, see the main [README](README.md) for supported languages, richer project configuration, affected-path exclusions, watch settings, and the diff --git a/README.md b/README.md index 70acbc5..1190235 100644 --- a/README.md +++ b/README.md @@ -355,6 +355,28 @@ first failure (`pytest -x`, `--bail`, `mix test --max-failures`, `go test -failfast`), when a target runs several commands, or when the runner cannot select single tests. +### Quarantining flaky tests + +With test results captured (`--test-results` or `--retry-failed`), a failed +target reports `passed (quarantined: N)` when, after retries, its only +failures are quarantined tests, its results are complete and it exited with +its runner's "tests failed" code. Its dependents run, and each such failure +gets a `::warning` annotation. The list comes from aster-srv (tests proven +flaky in 2 runs within 7 days) and from `aster-quarantine.toml` at the +workspace root, whose entries need a reason and an expiry date: + +```toml +[[test]] +name = "test creates an agent" +file = "test/platform/agents_test.exs" +reason = "DBConnection.OwnershipError under load; tracked in #4812" +expires = 2026-11-30 +``` + +`ASTER_QUARANTINE=off` turns it off. [Test tracking](docs/test-tracking.md) +covers capture per runner, uploading to aster-srv, the labels in the step +summary, and the quarantine rules in full. + ## Affected projects ```console diff --git a/crates/aster-results-model/src/ndjson.rs b/crates/aster-results-model/src/ndjson.rs index 051a73e..2d347f4 100644 --- a/crates/aster-results-model/src/ndjson.rs +++ b/crates/aster-results-model/src/ndjson.rs @@ -80,6 +80,7 @@ mod tests { duration_ms: Some(3), failure_hash: None, message: None, + quarantined: false, }), ] } diff --git a/crates/aster-results-model/src/record.rs b/crates/aster-results-model/src/record.rs index 0e4f48c..30c7e18 100644 --- a/crates/aster-results-model/src/record.rs +++ b/crates/aster-results-model/src/record.rs @@ -125,6 +125,14 @@ pub struct TestRecord { /// [`crate::scrub_message`] of the failure message. #[serde(default, skip_serializing_if = "Option::is_none")] pub message: Option, + /// Set on a test's last attempt when it failed but the test is + /// quarantined and Aster let its target pass. + #[serde(default, skip_serializing_if = "is_false")] + pub quarantined: bool, +} + +fn is_false(value: &bool) -> bool { + !value } #[cfg(test)] @@ -190,11 +198,20 @@ mod tests { duration_ms: Some(1840), failure_hash: Some("f3a9".into()), message: Some("** (DBConnection.OwnershipError) cannot find owner".into()), + quarantined: true, }); let json = serde_json::to_value(&record).unwrap(); assert_eq!(json["type"], "test"); assert_eq!(json["outcome"], "fail"); + assert_eq!(json["quarantined"], true); assert_eq!(round_trip(&record), record); + // Only a quarantined failure says so; older readers ignore it. + let Record::Test(mut plain) = record else { + unreachable!() + }; + plain.quarantined = false; + let json = serde_json::to_value(Record::Test(plain)).unwrap(); + assert!(json.get("quarantined").is_none()); } #[test] diff --git a/crates/aster-srv/Cargo.toml b/crates/aster-srv/Cargo.toml index ddd1954..62e17f3 100644 --- a/crates/aster-srv/Cargo.toml +++ b/crates/aster-srv/Cargo.toml @@ -27,6 +27,5 @@ tokio = { version = "1", features = ["macros", "net", "rt-multi-thread", "signal tokio-postgres = { version = "0.7", features = ["with-chrono-0_4"] } tokio-postgres-rustls = "0.14" gcp_auth = { version = "0.12.7", default-features = false, features = ["aws-lc-rs"] } - -[dev-dependencies] +# HMAC for webhook signatures; already in the build through rustls. aws-lc-rs = "1" diff --git a/crates/aster-srv/README.md b/crates/aster-srv/README.md index 0e18741..a48469c 100644 --- a/crates/aster-srv/README.md +++ b/crates/aster-srv/README.md @@ -21,12 +21,14 @@ from this server's state, looked up with the same credentials. | `GET /v1/test-results/tests/:test_id` | One test's state and history. | | `GET /v1/test-results/runs/:run_id/failures` | The failing tests of a run, with their state. | | `GET /v1/test-results/failures?github_run=` | The failing tests of every Aster run in one GitHub Actions run. | +| `GET /v1/test-results/quarantine` | The caller's quarantine list and its version. See [Quarantine](#quarantine). | | `GET /v1/test-results/durations?target=&day=` | Each test file's median duration, for splitting a partitioned suite. | | `GET /healthz` | The process is up. | | `GET /readyz` | The database answers. | Rows are kept for the retention period (30 days by default). Day partitions -past it are dropped every six hours. +past it are dropped every six hours, with the older quarantine log rows and +webhook deliveries. ## Flaky and broken tests @@ -71,6 +73,84 @@ branch matches it as `main` or `refs/heads/main`. (repo, `failure_hash`) the `failures` and distinct `tests` counts, `first_seen` and `last_seen` days, and an example message and test. +## Quarantine + +At the end of each pass, in the same transaction and as of the same clock, +the classifier updates `test_results.quarantine`, in this order: + +1. A quarantined test that is now **broken** leaves: broken tests are never + quarantined. +2. An entry older than `ASTER_SRV_QUARANTINE_DAYS` (default 30, at most the + retention period) **expires**. +3. A test whose final results since it entered end in + `ASTER_SRV_QUARANTINE_RELEASE_PASSES` (default 20) consecutive passes is + **released**. Any failure resets the count. +4. A test that is not broken **enters** once definite flake events from + `ASTER_SRV_QUARANTINE_RUNS` (default 2) different runs fall within the last + 7 days. Only events after it last left count, so a released or expired + test is judged afresh. `0` turns automatic quarantine off. + +Every change is a row in `test_results.quarantine_log` (`quarantined`, +`released`, `expired` or `broken`, with details), kept for the retention +period. A repository's newest log id is its list's `version`. + +`GET /v1/test-results/quarantine` (the upload token; `repo=` must match it) +returns: + +```json +{"repo": "ArchAstro/firstlanding", "version": 312, "generated_at": "…", + "truncated": false, + "tests": [{"test_id": "c81e…", "target": "//services/platform:test-ci", + "file": "test/platform/agents_test.exs", "suite": "Platform.AgentsTest", + "name": "test creates an agent", "since": "…", "expires_at": "…", + "runs": 2}]} +``` + +It lists at most 10,000 tests (`truncated` says when there were more), which +only lets more tests fail. `GET /v1/test-results/tests?state=quarantined` +filters the test list the same way. How Aster applies the list, and the +repository's own `aster-quarantine.toml`, are in +[docs/test-tracking.md](../../docs/test-tracking.md#5-quarantine). + +## Webhooks + +`ASTER_SRV_WEBHOOKS` is a JSON array of endpoints, each with the events it +wants: + +```json +[{"name": "flake-bot", "url": "https://hooks.example.com/aster", + "secret": "at-least-16-characters", "events": ["test.quarantined", "test.released"]}] +``` + +| Event | When | +|---|---| +| `test.quarantined` | A test entered quarantine. `detail`: `runs`, `events`, `expires_at`. | +| `test.released` | It passed enough times in a row. `detail`: `entered_at`, `passes`. | +| `test.expired` | Its entry reached the longest stay. `detail`: `entered_at`. | +| `test.broken` | A test became broken, quarantined or not. `detail`: `since_sha`, `failure_hash`. | + +Each delivery is a `POST` of + +```json +{"event": "test.quarantined", "occurred_at": "…", "repo": "ArchAstro/firstlanding", + "test": {"test_id": "…", "target": "…", "file": "…", "suite": "…", "name": "…"}, + "detail": {…}} +``` + +with `X-Aster-Event`, `X-Aster-Delivery` (the same on every retry, so a +receiver can drop repeats) and `X-Aster-Signature: sha256=`. Check the signature over the raw body before +parsing it. + +The classifier records deliveries in `webhooks.deliveries` in the transaction +that makes the change, so nothing is announced that did not happen and +nothing that happened is lost. A sender on every replica claims due rows +every 15 seconds (`FOR UPDATE SKIP LOCKED`) and posts them with a 10-second +timeout. A 2xx delivers; anything else is retried after 30 seconds, doubling +up to 6 hours, and abandoned after 12 attempts (about 14.5 hours). A slow or +failing endpoint holds up only its own deliveries. Endpoints must be `https://` +(`http://` only for localhost). Unsubscribed events, and all events with no +endpoints configured, record nothing. Rows are kept for the retention period. ## Reading results These routes take the same bearer token as uploads, and a token reads only its @@ -83,7 +163,7 @@ last classifier pass, up to five minutes old. Lists are paged with `limit` and an opaque `cursor`; pass the previous page's `next_cursor` until it is `null`. -`GET /v1/test-results/tests?state=flaky|broken|healthy&limit=100&cursor=` +`GET /v1/test-results/tests?state=flaky|broken|healthy|quarantined&limit=100&cursor=` (`limit` 1 to 500, default 100), sorted by `flake_score` descending, then `test_id`: @@ -273,7 +353,7 @@ needs no JavaScript. | `/ui` | Flaky leaderboard (flake score, last flake) and broken tests | | `/ui/tests/:test_id?repo=owner/name` | One test: state, a pass/fail strip (oldest to newest) and recent results with failure messages | | `/ui/clusters` | Failures of the last 7 days grouped by failure hash | -| `/ui/quarantine` | The quarantine list; empty until quarantine exists | +| `/ui/quarantine` | The quarantine list, its rules and its recent changes | `?repo=owner/name` picks the repository; without it the one with the newest run is shown. Any repository on `ASTER_SRV_REPOSITORIES` can be viewed. @@ -319,10 +399,25 @@ Pages are sent with a strict `Content-Security-Policy`, `nosniff`, | `ASTER_SRV_IAP_AUDIENCE` | unset | The Google IAP audience the dashboard accepts, exact or a prefix ending in `/`. Unset turns the dashboard off. See [Dashboard](#dashboard). | | `ASTER_SRV_UI_EMAIL_DOMAINS` | unset | Comma-separated domains (`example.com`, `*.example.com`) whose emails may view the dashboard. Unset allows anyone who passes IAP. | | `ASTER_SRV_UI_DEV_EMAIL` | unset | The viewer to trust when no IAP audience is set. Development only. | +| `ASTER_SRV_QUARANTINE_RUNS` | `2` | Different runs with a definite flake in 7 days that quarantine a test; `0` turns automatic quarantine off. | +| `ASTER_SRV_QUARANTINE_RELEASE_PASSES` | `20` | Consecutive passes that release a quarantined test. | +| `ASTER_SRV_QUARANTINE_DAYS` | `30` | The longest a test stays quarantined; at most `ASTER_SRV_RETENTION_DAYS`. | +| `ASTER_SRV_WEBHOOKS` | unset | Webhook endpoints, as JSON. See [Webhooks](#webhooks). | | `ASTER_SRV_STATIC_TOKENS` | unset | `owner/name=token` pairs accepted instead of OIDC tokens. Development only. | Logs are one JSON object per line, with `severity` and `message`. +## Runbook + +| Symptom | Check | Do | +|---|---|---| +| Uploads fail | `/readyz`; logs with `"severity":"ERROR"` | Uploads never fail CI, so results are only missing. Fix the database or the allowlist (`403`). | +| Classification stalls | `SELECT watermark FROM test_results.classifier_state` should be minutes old; `classifier pass failed` in the logs | A pass is safe to rerun; restarting a pod starts one at once. | +| A real regression is being let through | `/ui/quarantine`, the test's page | In CI set `ASTER_QUARANTINE=off` for the job to fail it at once. On the server, delete its row: `DELETE FROM test_results.quarantine WHERE repo = '…' AND test_id = '…'` (it can re-enter on new flakes after now). Set `ASTER_SRV_QUARANTINE_RUNS=0` to stop new entries. | +| Too many or too few tests quarantined | `SELECT action, count(*) FROM test_results.quarantine_log WHERE at > now() - interval '1 day' GROUP BY 1` | Tune `ASTER_SRV_QUARANTINE_RUNS` and `ASTER_SRV_QUARANTINE_RELEASE_PASSES`; they apply from the next pass. | +| Webhooks not arriving | `SELECT endpoint, event, attempts, last_status, last_error FROM webhooks.deliveries WHERE delivered_at IS NULL ORDER BY id DESC LIMIT 20` | Abandoned rows have `abandoned_at`; to resend, `UPDATE webhooks.deliveries SET abandoned_at = NULL, attempts = 0, next_attempt_at = now() WHERE id = …`. | +| A deploy waits at startup | `migration waiting for table locks` in the logs | Expected while the old release writes; it retries for about a minute. | + ## Image `ghcr.io/archastro/aster-srv:` is published with each Aster diff --git a/crates/aster-srv/src/config.rs b/crates/aster-srv/src/config.rs index dd7f5e8..fd1ac10 100644 --- a/crates/aster-srv/src/config.rs +++ b/crates/aster-srv/src/config.rs @@ -84,6 +84,26 @@ pub struct Config { /// `ASTER_SRV_UI_DEV_EMAIL`: the viewer the dashboard assumes when no IAP /// audience is configured. For local development only. pub ui_dev_email: Option, + /// When tests enter and leave quarantine. + pub quarantine: QuarantineRules, + /// `ASTER_SRV_WEBHOOKS`: where state changes are sent. Empty sends + /// nothing. + pub webhooks: Vec, +} + +/// When tests enter and leave quarantine. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct QuarantineRules { + /// `ASTER_SRV_QUARANTINE_RUNS`: a test enters quarantine once this many + /// different runs in the last 7 days hold a definite flake of it. 0 + /// turns automatic quarantine off. Default 2. + pub runs: u32, + /// `ASTER_SRV_QUARANTINE_RELEASE_PASSES`: consecutive passes, while + /// quarantined, that release a test. Default 20. + pub release_passes: u32, + /// `ASTER_SRV_QUARANTINE_DAYS`: the longest a test stays quarantined, + /// at most the retention period. Default 30. + pub days: u32, } /// A fixed bearer token standing in for one repository. @@ -173,6 +193,31 @@ impl Config { (or end in / to match any backend service of a project)" ); } + let number = |name: &str, default: u32, min: u32, max: u32| -> Result { + match var(name) { + None => Ok(default), + Some(value) => value + .trim() + .parse() + .ok() + .filter(|n| (min..=max).contains(n)) + .with_context(|| format!("{name} must be a number from {min} to {max}")), + } + }; + let quarantine = QuarantineRules { + runs: number("ASTER_SRV_QUARANTINE_RUNS", 2, 0, 1000)?, + release_passes: number("ASTER_SRV_QUARANTINE_RELEASE_PASSES", 20, 1, 10_000)?, + days: number( + "ASTER_SRV_QUARANTINE_DAYS", + 30.min(retention_days), + 1, + retention_days, + )?, + }; + let webhooks = match var("ASTER_SRV_WEBHOOKS") { + Some(value) => crate::webhooks::Endpoint::parse_all(&value)?, + None => Vec::new(), + }; Ok(Self { listen, database_url, @@ -188,6 +233,8 @@ impl Config { iap_audience, ui_email_domains, ui_dev_email: var("ASTER_SRV_UI_DEV_EMAIL").map(|email| email.trim().to_string()), + quarantine, + webhooks, }) } } @@ -306,6 +353,50 @@ mod tests { assert!(config.iap_audience.is_none()); assert!(config.ui_email_domains.is_none()); assert!(config.ui_dev_email.is_none()); + assert_eq!( + config.quarantine, + QuarantineRules { + runs: 2, + release_passes: 20, + days: 30 + } + ); + assert!(config.webhooks.is_empty()); + } + + #[test] + fn reads_the_quarantine_rules() { + let mut pairs = REQUIRED.to_vec(); + pairs.push(("ASTER_SRV_QUARANTINE_RUNS", "0")); + pairs.push(("ASTER_SRV_QUARANTINE_RELEASE_PASSES", "5")); + pairs.push(("ASTER_SRV_QUARANTINE_DAYS", "14")); + let config = Config::from_env(env_of(&pairs)).unwrap(); + assert_eq!( + config.quarantine, + QuarantineRules { + runs: 0, + release_passes: 5, + days: 14 + } + ); + // Quarantine never outlives the results that justify it. + let mut pairs = REQUIRED.to_vec(); + pairs.push(("ASTER_SRV_RETENTION_DAYS", "10")); + assert_eq!( + Config::from_env(env_of(&pairs)).unwrap().quarantine.days, + 10 + ); + pairs.push(("ASTER_SRV_QUARANTINE_DAYS", "11")); + assert!(Config::from_env(env_of(&pairs)).is_err()); + for (name, bad) in [ + ("ASTER_SRV_QUARANTINE_RUNS", "-1"), + ("ASTER_SRV_QUARANTINE_RELEASE_PASSES", "0"), + ("ASTER_SRV_QUARANTINE_DAYS", "0"), + ] { + let mut pairs = REQUIRED.to_vec(); + pairs.push((name, bad)); + assert!(Config::from_env(env_of(&pairs)).is_err(), "{name}={bad}"); + } } #[test] diff --git a/crates/aster-srv/src/lib.rs b/crates/aster-srv/src/lib.rs index 25cb3eb..ccc5630 100644 --- a/crates/aster-srv/src/lib.rs +++ b/crates/aster-srv/src/lib.rs @@ -15,6 +15,7 @@ pub mod log; pub mod raw_store; pub mod test_results; mod ui; +pub mod webhooks; /// An HTTP client builder on aws-lc-rs, the workspace's one TLS provider. /// reqwest is built without its own (it would bring ring, and two @@ -44,7 +45,10 @@ pub struct App { /// Every feature's migrations, in order. pub fn migrations() -> Vec<&'static db::Migration> { - test_results::MIGRATIONS.iter().collect() + test_results::MIGRATIONS + .iter() + .chain(webhooks::MIGRATIONS) + .collect() } impl App { diff --git a/crates/aster-srv/src/main.rs b/crates/aster-srv/src/main.rs index 2f2a156..677a3cf 100644 --- a/crates/aster-srv/src/main.rs +++ b/crates/aster-srv/src/main.rs @@ -41,6 +41,7 @@ async fn run() -> anyhow::Result<()> { let app = App::start(config).await?; aster_srv::test_results::retention::spawn(app.clone()); aster_srv::test_results::classifier::spawn(app.clone()); + aster_srv::webhooks::spawn(app.clone()); let listener = tokio::net::TcpListener::bind(listen).await?; log::info( "aster-srv listening", diff --git a/crates/aster-srv/src/test_results/classifier.rs b/crates/aster-srv/src/test_results/classifier.rs index c7bb38e..e2826cd 100644 --- a/crates/aster-srv/src/test_results/classifier.rs +++ b/crates/aster-srv/src/test_results/classifier.rs @@ -47,6 +47,10 @@ //! the last 7 days; `state` is `broken`, else `flaky` when the score is //! positive, else `healthy`. //! +//! Then the pass updates quarantine (see [`super::quarantine`]) and records +//! webhook notifications for what changed: a test entering, leaving or +//! expiring from quarantine, and a test becoming broken. +//! //! # Idempotence //! //! Events are keyed by (repo, test, kind, run) and inserted with @@ -61,7 +65,9 @@ use anyhow::Result; use chrono::{DateTime, NaiveDate, SecondsFormat, Utc}; use serde_json::json; -use super::ensure_partition; +use super::{ensure_partition, quarantine}; +use crate::config::QuarantineRules; +use crate::webhooks::{self, Notification}; use crate::App; const CLASSIFIER_LOCK: i64 = 0x6173_7465_722d_636c; // "aster-cl" @@ -88,6 +94,10 @@ pub struct Report { pub events: u64, /// `tests` rows whose state changed. pub tests_updated: u64, + /// Quarantine entries and exits. + pub quarantine: quarantine::Changes, + /// Webhook deliveries recorded. + pub deliveries: u64, } /// One pass as of `now`: classify every run received up to `now` that an @@ -101,6 +111,11 @@ pub async fn run_once(app: &App, now: DateTime) -> Result> { report.results += chunk.results; report.events += chunk.events; report.tests_updated += chunk.tests_updated; + report.quarantine.quarantined += chunk.quarantine.quarantined; + report.quarantine.released += chunk.quarantine.released; + report.quarantine.expired += chunk.quarantine.expired; + report.quarantine.broken += chunk.quarantine.broken; + report.deliveries += chunk.deliveries; if done { return Ok(Some(report)); } @@ -118,6 +133,11 @@ pub fn spawn(app: std::sync::Arc) { "results": report.results, "events": report.events, "tests_updated": report.tests_updated, + "quarantined": report.quarantine.quarantined, + "released": report.quarantine.released, + "expired": report.quarantine.expired, + "broken_in_quarantine": report.quarantine.broken, + "deliveries": report.deliveries, }), ), Ok(None) => {} @@ -150,18 +170,26 @@ fn text(value: &str) -> String { } /// What one transaction fixes: the window, the clock and the settings. -struct Window { - /// Runs received in `(from, to]` are classified. +pub(super) struct Window { + /// Runs received in `(from, to]` are classified; `to` is the pass's + /// clock. from: DateTime, - to: DateTime, + pub(super) to: DateTime, /// The oldest day whose results count as history. history_from: NaiveDate, /// The branch names that mean the default branch. on_default: String, broken_runs: u32, + quarantine: QuarantineRules, + webhooks: Vec, } impl Window { + /// `to` as a SQL literal. + pub(super) fn to_literal(&self) -> String { + at(self.to) + } + fn branch_is_default(&self, branch: &str) -> String { format!( "{branch} IN ({0}, {1})", @@ -172,7 +200,7 @@ impl Window { /// Each run's final result of one test: `filter` picks the test and /// bounds the runs. `strict` leaves out `partial` targets. - fn finals(&self, filter: &str, strict: bool) -> String { + pub(super) fn finals(&self, filter: &str, strict: bool) -> String { format!( "SELECT DISTINCT ON (tr.run_id) tr.run_id, r.sha, r.received_at, \ tr.outcome IN ('fail', 'error') AS is_fail, tr.failure_hash \ @@ -220,6 +248,8 @@ async fn pass(app: &App, now: DateTime) -> Result> { history_from: now.date_naive() - chrono::Days::new(u64::from(app.config.retention_days)), on_default: app.config.default_branch.clone(), broken_runs: app.config.broken_runs, + quarantine: app.config.quarantine, + webhooks: app.config.webhooks.clone(), }; // Event partitions for the days being classified. Another replica may @@ -238,6 +268,19 @@ async fn pass(app: &App, now: DateTime) -> Result> { if !locked { return Ok(None); } + // Under the lock, a pass whose clock is behind what another replica + // committed would re-derive state, quarantine and notifications as of an + // earlier time: a broken test could turn healthy and be announced as + // breaking again. That replica covered this window: skip it, and go on + // from its watermark if `now` is later still. + let committed: Option> = transaction + .query_opt("SELECT watermark FROM test_results.classifier_state", &[]) + .await? + .map(|row| row.get(0)); + if let Some(committed) = committed.filter(|committed| *committed > window.to) { + transaction.rollback().await?; + return Ok(Some((Report::default(), committed >= now))); + } let report = classify(&transaction, &window).await?; transaction .execute( @@ -405,7 +448,7 @@ async fn classify( true, ); let tests_updated = transaction - .execute( + .query( &format!( "WITH affected AS ( \ SELECT repo, test_id FROM cls_win \ @@ -445,8 +488,9 @@ async fn classify( derived AS ( \ SELECT s.repo, s.test_id, s.score, s.last_flake_at, \ COALESCE(k.length, 0) >= {k} AS broken, \ - k.since_sha, k.failure_hash \ - FROM scores s LEFT JOIN streaks k USING (repo, test_id)) \ + k.since_sha, k.failure_hash, o.state AS old_state \ + FROM scores s LEFT JOIN streaks k USING (repo, test_id) \ + JOIN test_results.tests o USING (repo, test_id)) \ UPDATE test_results.tests t SET \ state = CASE WHEN d.broken THEN 'broken' \ WHEN d.score > 0 THEN 'flaky' ELSE 'healthy' END, \ @@ -463,18 +507,47 @@ async fn classify( WHEN d.score > 0 THEN 'flaky' ELSE 'healthy' END, \ d.score, d.last_flake_at, \ CASE WHEN d.broken THEN d.since_sha END, \ - CASE WHEN d.broken THEN d.failure_hash END)", + CASE WHEN d.broken THEN d.failure_hash END) \ + RETURNING t.repo, t.test_id, t.target, t.file, t.suite, t.name, t.state, \ + d.old_state, t.broken_since_sha, t.broken_failure_hash", to = at(window.to), k = window.broken_runs, ), &[], ) .await?; + let mut notifications: Vec = tests_updated + .iter() + .filter(|row| { + row.get::<_, String>("state") == "broken" + && row.get::<_, String>("old_state") != "broken" + }) + .map(|row| { + quarantine::notification( + "test.broken", + &row.get::<_, String>("repo"), + window.to, + quarantine::test_json(row), + json!({ + "since_sha": row.get::<_, Option>("broken_since_sha"), + "failure_hash": row.get::<_, Option>("broken_failure_hash"), + }), + ) + }) + .collect(); + + let (quarantine, mut changed) = + quarantine::update(transaction, window, window.quarantine).await?; + notifications.append(&mut changed); + let deliveries = + webhooks::enqueue(transaction, &window.webhooks, ¬ifications, window.to).await?; Ok(Report { results, events, - tests_updated, + tests_updated: tests_updated.len() as u64, + quarantine, + deliveries, }) } diff --git a/crates/aster-srv/src/test_results/mod.rs b/crates/aster-srv/src/test_results/mod.rs index ae9b6a0..5d91cee 100644 --- a/crates/aster-srv/src/test_results/mod.rs +++ b/crates/aster-srv/src/test_results/mod.rs @@ -7,11 +7,13 @@ //! received, so retention drops whole days. `tests` holds each test's //! identity, when it was first and last seen, and what the classifier //! concluded about it. `test_flake_events` (also day-partitioned) is the -//! classifier's evidence. +//! classifier's evidence. `quarantine` lists the tests held out of failing +//! builds, and `quarantine_log` how that list changed. pub mod classifier; pub mod durations; pub mod ingest; +pub mod quarantine; pub mod query; pub mod retention; @@ -161,6 +163,12 @@ CREATE INDEX test_results_failures ON test_results.test_results (repo, received_ WHERE outcome IN ('fail', 'error') AND failure_hash IS NOT NULL; "#, }, + Migration { + feature: "test_results", + version: 4, + // New tables only. + sql: quarantine::MIGRATION_SQL, + }, ]; /// The day-partitioned tables. diff --git a/crates/aster-srv/src/test_results/quarantine.rs b/crates/aster-srv/src/test_results/quarantine.rs new file mode 100644 index 0000000..0ef787b --- /dev/null +++ b/crates/aster-srv/src/test_results/quarantine.rs @@ -0,0 +1,285 @@ +//! Quarantine: tests proven flaky enough that Aster stops letting them fail +//! the build. +//! +//! The classifier keeps `quarantine` current at the end of each pass, in the +//! pass's transaction and as of its clock, after it has re-derived every +//! test's state: +//! +//! 1. A quarantined test that is now `broken` leaves at once: broken tests +//! are never quarantined. +//! 2. An entry older than `ASTER_SRV_QUARANTINE_DAYS` expires. +//! 3. A test whose final results since it entered end in +//! `ASTER_SRV_QUARANTINE_RELEASE_PASSES` consecutive passes is released. +//! Skips and cached targets' replayed results do not count. +//! 4. A test that is not broken enters when definite flake events from +//! `ASTER_SRV_QUARANTINE_RUNS` different runs fall in the last 7 days. +//! Only events after the test last left count, so a released or expired +//! test is judged afresh. +//! +//! Every change is a row in `quarantine_log`, kept for the retention period. +//! The newest log id of a repository is its quarantine list's version. + +use anyhow::Result; +use chrono::{DateTime, Utc}; +use serde_json::{json, Value}; + +use super::classifier::Window; +use crate::config::QuarantineRules; +use crate::webhooks::Notification; + +/// Days of definite events that count toward entering. +const ENTRY_DAYS: i64 = 7; + +pub const MIGRATION_SQL: &str = r#" +CREATE TABLE test_results.quarantine ( + repo text NOT NULL, + test_id text NOT NULL, + entered_at timestamptz NOT NULL, + expires_at timestamptz NOT NULL, + runs integer NOT NULL, + PRIMARY KEY (repo, test_id) +); + +CREATE TABLE test_results.quarantine_log ( + id bigserial PRIMARY KEY, + repo text NOT NULL, + test_id text NOT NULL, + action text NOT NULL + CHECK (action IN ('quarantined', 'released', 'expired', 'broken')), + at timestamptz NOT NULL, + detail jsonb NOT NULL +); +CREATE INDEX quarantine_log_test ON test_results.quarantine_log (repo, test_id, at); +CREATE INDEX quarantine_log_repo ON test_results.quarantine_log (repo, id); +CREATE INDEX quarantine_log_at ON test_results.quarantine_log (at); +"#; + +/// What one pass changed. +#[derive(Debug, Default, PartialEq, Eq)] +pub struct Changes { + pub quarantined: u64, + pub released: u64, + pub expired: u64, + /// Quarantined tests that became broken. + pub broken: u64, +} + +/// A log row as the classifier wrote it, with the test it names. +struct Logged { + action: String, + repo: String, + at: DateTime, + detail: Value, + test: Value, +} + +/// Bring quarantine up to date as of `window.to`. Returns what changed and +/// the notifications to send for it. +pub(super) async fn update( + transaction: &tokio_postgres::Transaction<'_>, + window: &Window, + rules: QuarantineRules, +) -> Result<(Changes, Vec)> { + let to = window.to_literal(); + // Days here are 24 hours. `interval 'N days'` is calendar arithmetic in + // the session's time zone and gains or loses an hour across a DST change. + let mut ids: Vec = Vec::new(); + let mut log = |rows: Vec| { + ids.extend(rows.iter().map(|row| row.get::<_, i64>(0))); + rows.len() as u64 + }; + + // 1. Broken tests leave. + let broken = log(transaction + .query( + &format!( + "WITH gone AS ( \ + DELETE FROM test_results.quarantine q USING test_results.tests t \ + WHERE t.repo = q.repo AND t.test_id = q.test_id AND t.state = 'broken' \ + RETURNING q.repo, q.test_id, q.entered_at, t.broken_since_sha) \ + INSERT INTO test_results.quarantine_log (repo, test_id, action, at, detail) \ + SELECT repo, test_id, 'broken', {to}, \ + jsonb_build_object('entered_at', entered_at, \ + 'broken_since_sha', broken_since_sha) \ + FROM gone RETURNING id" + ), + &[], + ) + .await?); + + // 2. Old entries expire. + let expired = log(transaction + .query( + &format!( + "WITH gone AS ( \ + DELETE FROM test_results.quarantine WHERE expires_at <= {to} \ + RETURNING repo, test_id, entered_at) \ + INSERT INTO test_results.quarantine_log (repo, test_id, action, at, detail) \ + SELECT repo, test_id, 'expired', {to}, \ + jsonb_build_object('entered_at', entered_at) \ + FROM gone RETURNING id" + ), + &[], + ) + .await?); + + // 3. A run of passes since entering releases. Each run's final attempt + // counts, with the classifier's rules: cached targets store the results + // they replay, and a replayed pass proves nothing. + let since_entry = window.finals( + &format!( + "tr.repo = q.repo AND tr.test_id = q.test_id \ + AND tr.received_on >= (q.entered_at AT TIME ZONE 'UTC')::date \ + AND r.received_at > q.entered_at AND r.received_at <= {to}" + ), + false, + ); + let released = log(transaction + .query( + &format!( + "WITH streaks AS ( \ + SELECT q.repo, q.test_id, q.entered_at, \ + (SELECT count(*) FROM ( \ + SELECT bool_and(NOT f.is_fail) OVER ( \ + ORDER BY f.received_at DESC, f.run_id DESC) AS clean \ + FROM ({since_entry}) f) s \ + WHERE s.clean) AS passes \ + FROM test_results.quarantine q), \ + gone AS ( \ + DELETE FROM test_results.quarantine q USING streaks s \ + WHERE s.repo = q.repo AND s.test_id = q.test_id \ + AND s.passes >= {passes} \ + RETURNING q.repo, q.test_id, q.entered_at, s.passes) \ + INSERT INTO test_results.quarantine_log (repo, test_id, action, at, detail) \ + SELECT repo, test_id, 'released', {to}, \ + jsonb_build_object('entered_at', entered_at, 'passes', passes) \ + FROM gone RETURNING id", + passes = rules.release_passes, + ), + &[], + ) + .await?); + + // 4. Proven flakes enter. + let mut quarantined = 0; + if rules.runs > 0 { + let from_day = (window.to - chrono::Duration::days(ENTRY_DAYS)).date_naive(); + quarantined = log(transaction + .query( + &format!( + "WITH left_at AS ( \ + SELECT repo, test_id, max(at) AS at FROM test_results.quarantine_log \ + WHERE action <> 'quarantined' GROUP BY repo, test_id), \ + proven AS ( \ + SELECT e.repo, e.test_id, count(DISTINCT e.run_id)::int AS runs, \ + count(*)::int AS events \ + FROM test_results.test_flake_events e \ + LEFT JOIN left_at l ON l.repo = e.repo AND l.test_id = e.test_id \ + WHERE e.kind = 'definite' AND e.observed_on >= '{from_day}'::date \ + AND e.observed_at > {to} - {ENTRY_DAYS} * interval '24 hours' \ + AND e.observed_at <= {to} \ + AND (l.at IS NULL OR e.observed_at > l.at) \ + GROUP BY e.repo, e.test_id \ + HAVING count(DISTINCT e.run_id) >= {runs}), \ + entered AS ( \ + INSERT INTO test_results.quarantine \ + (repo, test_id, entered_at, expires_at, runs) \ + SELECT p.repo, p.test_id, {to}, {to} + {days} * interval '24 hours', p.runs \ + FROM proven p \ + JOIN test_results.tests t \ + ON t.repo = p.repo AND t.test_id = p.test_id \ + AND t.state <> 'broken' \ + ON CONFLICT (repo, test_id) DO NOTHING \ + RETURNING repo, test_id, runs, expires_at) \ + INSERT INTO test_results.quarantine_log (repo, test_id, action, at, detail) \ + SELECT n.repo, n.test_id, 'quarantined', {to}, \ + jsonb_build_object('runs', n.runs, 'events', p.events, \ + 'expires_at', n.expires_at) \ + FROM entered n JOIN proven p USING (repo, test_id) RETURNING id", + runs = rules.runs, + days = rules.days, + ), + &[], + ) + .await?); + } + + let changes = Changes { + quarantined, + released, + expired, + broken, + }; + let logged = transaction + .query( + "SELECT l.action, l.repo, l.at, l.detail::text AS detail, l.test_id, t.target, t.file, t.suite, t.name \ + FROM test_results.quarantine_log l \ + LEFT JOIN test_results.tests t ON t.repo = l.repo AND t.test_id = l.test_id \ + WHERE l.id = ANY($1) ORDER BY l.id", + &[&ids], + ) + .await? + .iter() + .map(|row| Logged { + action: row.get("action"), + repo: row.get("repo"), + at: row.get("at"), + detail: serde_json::from_str(row.get("detail")).unwrap_or(Value::Null), + test: test_json(row), + }) + .collect::>(); + let notifications = logged + .into_iter() + .filter_map(|entry| { + // A quarantined test turning broken is announced as test.broken, + // with every other test that broke. + let event = match entry.action.as_str() { + "quarantined" => "test.quarantined", + "released" => "test.released", + "expired" => "test.expired", + _ => return None, + }; + Some(notification( + event, + &entry.repo, + entry.at, + entry.test, + entry.detail, + )) + }) + .collect(); + Ok((changes, notifications)) +} + +/// The test fields of a row holding `test_id`, `target`, `file`, `suite` +/// and `name`. +pub(super) fn test_json(row: &tokio_postgres::Row) -> Value { + json!({ + "test_id": row.get::<_, String>("test_id"), + "target": row.get::<_, Option>("target"), + "file": row.get::<_, Option>("file"), + "suite": row.get::<_, Option>("suite"), + "name": row.get::<_, Option>("name"), + }) +} + +/// A webhook body: the event, when it happened, the repository, the test +/// and what the event adds. +pub(super) fn notification( + event: &'static str, + repo: &str, + at: DateTime, + test: Value, + detail: Value, +) -> Notification { + Notification { + event, + body: json!({ + "event": event, + "occurred_at": at.to_rfc3339_opts(chrono::SecondsFormat::Millis, true), + "repo": repo, + "test": test, + "detail": detail, + }), + } +} diff --git a/crates/aster-srv/src/test_results/query.rs b/crates/aster-srv/src/test_results/query.rs index ee96bab..d4c00d5 100644 --- a/crates/aster-srv/src/test_results/query.rs +++ b/crates/aster-srv/src/test_results/query.rs @@ -1,7 +1,8 @@ //! The read API over test state: `GET /v1/test-results/tests`, -//! `.../tests/:test_id`, `.../runs/:run_id/failures` and +//! `.../tests/:test_id`, `.../runs/:run_id/failures`, //! `.../failures?github_run=` (the failures of every Aster run in one GitHub -//! Actions run). +//! Actions run) and `.../quarantine` (the tests Aster keeps from failing +//! builds). //! //! A caller reads only its own repository's data. Lists page with an opaque //! keyset cursor (hex of the last row's sort key), so a page costs the same @@ -20,6 +21,9 @@ const DEFAULT_LIMIT: i64 = 100; const MAX_LIMIT: i64 = 500; /// The most failures one run's report lists in a page. const MAX_FAILURES: i64 = 1000; +/// The most tests the quarantine list returns; past it the list is cut +/// (and says so), which only lets more tests fail. +const MAX_QUARANTINED: i64 = 10_000; /// A read route. #[derive(Debug, PartialEq, Eq)] @@ -28,6 +32,7 @@ pub enum Route { Test(String), RunFailures(String), GithubFailures, + Quarantine, } /// The route for a request path, with ids percent-decoded (a run id holds @@ -38,6 +43,7 @@ pub fn parse_route(path: &str) -> Option { match segments.as_slice() { ["tests"] => Some(Route::Tests), ["failures"] => Some(Route::GithubFailures), + ["quarantine"] => Some(Route::Quarantine), ["tests", id] if !id.is_empty() => Some(Route::Test(percent_decode(id)?)), ["runs", id, "failures"] if !id.is_empty() => Some(Route::RunFailures(percent_decode(id)?)), _ => None, @@ -156,8 +162,11 @@ fn params( ) } "state" => { - if !["flaky", "broken", "healthy"].contains(&value.as_str()) { - return Err(Reply::error(400, "state must be flaky, broken or healthy")); + if !["flaky", "broken", "healthy", "quarantined"].contains(&value.as_str()) { + return Err(Reply::error( + 400, + "state must be flaky, broken, healthy or quarantined", + )); } found.state = Some(value); } @@ -191,6 +200,7 @@ pub async fn handle(app: &App, identity: &Identity, route: Route, query: Option< Route::Test(test_id) => test_detail(app, identity, &test_id, query).await, Route::RunFailures(run_id) => run_failures(app, identity, &run_id, query).await, Route::GithubFailures => github_failures(app, identity, query).await, + Route::Quarantine => quarantine_list(app, identity, query).await, }; match result { Ok(reply) | Err(Failure::Reply(reply)) => reply, @@ -246,12 +256,17 @@ fn test_json(row: &Row) -> Value { /// The list query: `repo` is `$1`; a state filter is `$2`; a cursor is the /// next two parameters (score, test id); the limit is last. Sorted along -/// `tests_by_score` / `tests_by_state_score`. +/// `tests_by_score` / `tests_by_state_score`. The state `quarantined` is +/// not a column but membership of the quarantine list, so its `$2` is +/// matched by an `EXISTS` on `quarantine` instead. pub fn list_sql(with_state: bool, with_cursor: bool) -> String { let mut next = 2; let mut filters = String::new(); if with_state { - filters.push_str(&format!(" AND state = ${next}")); + filters.push_str(&format!( + " AND (CASE WHEN ${next} = 'quarantined' THEN {} ELSE state = ${next} END)", + quarantine_filter() + )); next += 1; } if with_cursor { @@ -267,6 +282,67 @@ pub fn list_sql(with_state: bool, with_cursor: bool) -> String { ) } +/// Whether the `tests` row is on the quarantine list. +fn quarantine_filter() -> &'static str { + "EXISTS (SELECT 1 FROM test_results.quarantine q \ + WHERE q.repo = tests.repo AND q.test_id = tests.test_id)" +} + +/// `GET /v1/test-results/quarantine`: every quarantined test of the +/// caller's repository, and the list's version (its newest change). +async fn quarantine_list(app: &App, identity: &Identity, query: Option<&str>) -> Handled { + let params = params(identity, query, 1, 1, false)?; + if params.state.is_some() || params.cursor.is_some() || query.unwrap_or("").contains("limit") { + return Err(Reply::error(400, "this route takes only repo").into()); + } + let client = app.pool.get().await?; + let version: i64 = client + .query_one( + "SELECT COALESCE(max(id), 0) FROM test_results.quarantine_log WHERE repo = $1", + &[¶ms.repo], + ) + .await? + .get(0); + let fetch = MAX_QUARANTINED + 1; + let rows = client + .query( + "SELECT q.test_id, q.entered_at, q.expires_at, q.runs, \ + t.target, t.file, t.suite, t.name \ + FROM test_results.quarantine q \ + LEFT JOIN test_results.tests t ON t.repo = q.repo AND t.test_id = q.test_id \ + WHERE q.repo = $1 ORDER BY q.test_id LIMIT $2", + &[¶ms.repo, &fetch], + ) + .await?; + let truncated = rows.len() as i64 > MAX_QUARANTINED; + let tests: Vec = rows + .iter() + .take(MAX_QUARANTINED as usize) + .map(|row| { + json!({ + "test_id": row.get::<_, String>("test_id"), + "target": row.get::<_, Option>("target"), + "file": row.get::<_, Option>("file"), + "suite": row.get::<_, Option>("suite"), + "name": row.get::<_, Option>("name"), + "since": time(row.get("entered_at")), + "expires_at": time(row.get("expires_at")), + "runs": row.get::<_, i32>("runs"), + }) + }) + .collect(); + Ok(Reply::json( + 200, + json!({ + "repo": params.repo, + "version": version, + "generated_at": time(Utc::now()), + "tests": tests, + "truncated": truncated, + }), + )) +} + async fn list_tests(app: &App, identity: &Identity, query: Option<&str>) -> Handled { let params = params(identity, query, MAX_LIMIT, DEFAULT_LIMIT, false)?; let cursor = match ¶ms.cursor { diff --git a/crates/aster-srv/src/test_results/retention.rs b/crates/aster-srv/src/test_results/retention.rs index 8c4fd39..cd08169 100644 --- a/crates/aster-srv/src/test_results/retention.rs +++ b/crates/aster-srv/src/test_results/retention.rs @@ -1,5 +1,6 @@ //! Keeping the retention period: whole day partitions older than it are -//! dropped, and runs and tests not seen within it are deleted. +//! dropped, and runs and tests not seen within it are deleted, as are older +//! quarantine log rows and webhook deliveries. use std::time::Duration; @@ -18,6 +19,8 @@ pub struct Report { pub dropped: Vec, pub runs_deleted: u64, pub tests_deleted: u64, + /// Quarantine log rows and webhook deliveries. + pub log_deleted: u64, } /// One pass as of `today`: create the coming days' partitions, then remove @@ -72,6 +75,18 @@ pub async fn run_once(app: &App, today: NaiveDate) -> Result> { &[&cutoff_at], ) .await?; + report.log_deleted = transaction + .execute( + "DELETE FROM test_results.quarantine_log WHERE at < $1", + &[&cutoff_at], + ) + .await? + + transaction + .execute( + "DELETE FROM webhooks.deliveries WHERE created_at < $1", + &[&cutoff_at], + ) + .await?; transaction.commit().await?; Ok(Some(report)) } @@ -87,6 +102,7 @@ pub fn spawn(app: std::sync::Arc) { "dropped": report.dropped, "runs_deleted": report.runs_deleted, "tests_deleted": report.tests_deleted, + "log_deleted": report.log_deleted, }), ), Ok(None) => {} diff --git a/crates/aster-srv/src/ui/data.rs b/crates/aster-srv/src/ui/data.rs index 7dfe3ec..61bd65f 100644 --- a/crates/aster-srv/src/ui/data.rs +++ b/crates/aster-srv/src/ui/data.rs @@ -11,6 +11,8 @@ pub const FLAKY_LIMIT: i64 = 200; pub const SCORED_LIMIT: i64 = 100; pub const BROKEN_LIMIT: i64 = 100; pub const CLUSTER_LIMIT: i64 = 50; +pub const QUARANTINE_LIMIT: i64 = 500; +pub const QUARANTINE_LOG_LIMIT: i64 = 50; /// The most results one test's page reads: the strip shows all of them, the /// table the newest [`TABLE_ROWS`]. pub const HISTORY_LIMIT: i64 = 300; @@ -222,3 +224,68 @@ pub async fn clusters(client: &Client, repo: &str, since: NaiveDate) -> Result, + pub target: Option, + pub file: Option, + pub since: DateTime, + pub expires_at: DateTime, + pub runs: i32, +} + +/// One change to the quarantine list. +pub struct QuarantineChange { + pub test_id: String, + pub name: Option, + pub action: String, + pub at: DateTime, +} + +/// The quarantine list, oldest entry first, and its newest changes. +pub async fn quarantine( + client: &Client, + repo: &str, +) -> Result<(Vec, Vec)> { + let entries = client + .query( + "SELECT q.test_id, q.entered_at, q.expires_at, q.runs, t.name, t.target, t.file \ + FROM test_results.quarantine q \ + LEFT JOIN test_results.tests t ON t.repo = q.repo AND t.test_id = q.test_id \ + WHERE q.repo = $1 ORDER BY q.entered_at, q.test_id LIMIT $2", + &[&repo, &QUARANTINE_LIMIT], + ) + .await? + .iter() + .map(|row| Quarantined { + test_id: row.get("test_id"), + name: row.get("name"), + target: row.get("target"), + file: row.get("file"), + since: row.get("entered_at"), + expires_at: row.get("expires_at"), + runs: row.get("runs"), + }) + .collect(); + let changes = client + .query( + "SELECT l.test_id, l.action, l.at, t.name \ + FROM test_results.quarantine_log l \ + LEFT JOIN test_results.tests t ON t.repo = l.repo AND t.test_id = l.test_id \ + WHERE l.repo = $1 ORDER BY l.id DESC LIMIT $2", + &[&repo, &QUARANTINE_LOG_LIMIT], + ) + .await? + .iter() + .map(|row| QuarantineChange { + test_id: row.get("test_id"), + name: row.get("name"), + action: row.get("action"), + at: row.get("at"), + }) + .collect(); + Ok((entries, changes)) +} diff --git a/crates/aster-srv/src/ui/mod.rs b/crates/aster-srv/src/ui/mod.rs index be4f1a3..44fe5bc 100644 --- a/crates/aster-srv/src/ui/mod.rs +++ b/crates/aster-srv/src/ui/mod.rs @@ -4,7 +4,7 @@ //! - `/ui`: the flaky leaderboard and broken tests //! - `/ui/tests/:test_id`: one test's state and history //! - `/ui/clusters`: failures grouped by failure hash -//! - `/ui/quarantine`: the quarantine list (not built yet) +//! - `/ui/quarantine`: the quarantine list and its recent changes //! //! `?repo=owner/name` picks the repository; without it the one with the //! newest run is shown. Any repository on the allowlist can be viewed. The @@ -170,7 +170,7 @@ async fn render( Route::Test(_) => ("Test", Section::Other), }; let body = if route == Route::Quarantine { - pages::quarantine() + pages::quarantine(None, app.config.quarantine, &[], &[]) } else { pages::no_runs() }; @@ -195,7 +195,14 @@ async fn render( &pages::clusters(repo_name, &clusters), ) } - Route::Quarantine => make("Quarantine", Section::Quarantine, &pages::quarantine()), + Route::Quarantine => { + let (entries, changes) = data::quarantine(&client, repo_name).await?; + make( + "Quarantine", + Section::Quarantine, + &pages::quarantine(Some(repo_name), app.config.quarantine, &entries, &changes), + ) + } Route::Test(test_id) => { let Some(test) = data::test(&client, repo_name, &test_id).await? else { let body = "

No such test in this repository.

"; diff --git a/crates/aster-srv/src/ui/pages.rs b/crates/aster-srv/src/ui/pages.rs index 9bfac86..a448c9e 100644 --- a/crates/aster-srv/src/ui/pages.rs +++ b/crates/aster-srv/src/ui/pages.rs @@ -4,10 +4,12 @@ use std::fmt::Write; use super::data::{ - Attempt, Cluster, Leaderboard, TestRow, BROKEN_LIMIT, CLUSTER_DAYS, CLUSTER_LIMIT, FLAKY_LIMIT, - HISTORY_LIMIT, SCORED_LIMIT, TABLE_ROWS, + Attempt, Cluster, Leaderboard, QuarantineChange, Quarantined, TestRow, BROKEN_LIMIT, + CLUSTER_DAYS, CLUSTER_LIMIT, FLAKY_LIMIT, HISTORY_LIMIT, QUARANTINE_LIMIT, SCORED_LIMIT, + TABLE_ROWS, }; use super::html::{escape, short_sha, test_link, when, when_opt}; +use crate::config::QuarantineRules; /// Shown on every page when no repository has runs. pub fn no_runs() -> String { @@ -275,12 +277,96 @@ pub fn clusters(repo: &str, clusters: &[Cluster]) -> String { out } -/// Quarantine is not built yet (task T22 fills `#quarantine`). -pub fn quarantine() -> String { - "
\ -

Quarantine is not enabled yet. When it is, tests that are held out of \ - the required checks will be listed here.

" - .to_string() +/// How tests enter and leave quarantine, in one paragraph. +fn rules_note(rules: QuarantineRules) -> String { + let entry = if rules.runs == 0 { + "Automatic quarantine is off.".to_string() + } else { + format!( + "A test enters after definite flakes in {} different run{} within 7 days, unless it \ + is broken.", + rules.runs, + if rules.runs == 1 { "" } else { "s" } + ) + }; + format!( + "

Quarantined tests still run and upload, but their failures do not \ + fail the build. {entry} It leaves after {} consecutive passes, when it breaks, or after \ + {} days. Entries in a repository's aster-quarantine.toml also apply and are \ + not listed here.

", + rules.release_passes, rules.days + ) +} + +/// The quarantine list (`#quarantine`) and its recent changes. +pub fn quarantine( + repo: Option<&str>, + rules: QuarantineRules, + entries: &[Quarantined], + changes: &[QuarantineChange], +) -> String { + let mut out = String::from("
"); + out.push_str(&rules_note(rules)); + let name_cell = |test_id: &str, name: Option<&str>| match repo { + Some(repo) => format!( + "{}
{}", + escape(&test_link(test_id, repo)), + escape(name.unwrap_or(test_id)), + escape(test_id), + ), + None => format!("{}", escape(test_id)), + }; + if entries.is_empty() { + out.push_str("

No tests are quarantined.

"); + } else { + out.push_str( + "
\ + ", + ); + for entry in entries { + let file = entry + .file + .as_deref() + .map(|f| format!("
{}", escape(f))) + .unwrap_or_default(); + let _ = write!( + out, + "{}\ + ", + name_cell(&entry.test_id, entry.name.as_deref()), + escape(&when(entry.since)), + escape(&when(entry.expires_at)), + entry.runs, + escape(entry.target.as_deref().unwrap_or("-")), + file, + ); + } + out.push_str("
TestSinceExpiresRunsTarget / file
{}{}{}{}{}
"); + out.push_str(&capped(entries.len(), QUARANTINE_LIMIT)); + } + if !changes.is_empty() { + out.push_str( + "

Recent changes

\ + ", + ); + for change in changes { + let class = match change.action.as_str() { + "quarantined" => "flaky", + "broken" => "broken", + _ => "healthy", + }; + let _ = write!( + out, + "{}", + escape(&when(change.at)), + escape(&change.action), + name_cell(&change.test_id, change.name.as_deref()), + ); + } + out.push_str("
WhenChangeTest
{}{}
"); + } + out.push_str("
"); + out } #[cfg(test)] @@ -330,9 +416,45 @@ mod tests { assert!(message(None).contains("muted")); } + const RULES: QuarantineRules = QuarantineRules { + runs: 2, + release_passes: 20, + days: 30, + }; + + #[test] + fn quarantine_page_has_an_empty_state_and_states_the_rules() { + let html = quarantine(Some("o/r"), RULES, &[], &[]); + assert!(html.contains("No tests are quarantined") && html.contains("id=\"quarantine\"")); + assert!(html.contains("2 different runs") && html.contains("20 consecutive passes")); + let off = quarantine(None, QuarantineRules { runs: 0, ..RULES }, &[], &[]); + assert!(off.contains("Automatic quarantine is off")); + } + #[test] - fn quarantine_page_has_an_empty_state() { - let html = quarantine(); - assert!(html.contains("not enabled yet") && html.contains("id=\"quarantine\"")); + fn quarantine_page_escapes_and_links_tests() { + let at = Utc.with_ymd_and_hms(2026, 10, 13, 14, 3, 0).unwrap(); + let html = quarantine( + Some("o/r"), + RULES, + &[Quarantined { + test_id: "".into(), + name: Some("n".into()), + target: Some("//a:t".into()), + file: None, + since: at, + expires_at: at, + runs: 3, + }], + &[QuarantineChange { + test_id: "gone".into(), + name: None, + action: "released".into(), + at, + }], + ); + assert!(!html.contains("") && !html.contains("")); + assert!(html.contains("/ui/tests/%3Cid%3E?repo=o%2Fr")); + assert!(html.contains("Recent changes") && html.contains("released")); } } diff --git a/crates/aster-srv/src/webhooks.rs b/crates/aster-srv/src/webhooks.rs new file mode 100644 index 0000000..e06e547 --- /dev/null +++ b/crates/aster-srv/src/webhooks.rs @@ -0,0 +1,440 @@ +//! Webhook notifications: configured endpoints hear about tests changing +//! state. +//! +//! The classifier records each notification in `webhooks.deliveries` in the +//! same transaction as the change it reports (an outbox), one row per +//! subscribed endpoint, so a change is never announced without happening +//! nor lost after it happens. A separate loop sends due rows and retries +//! failures with exponential backoff; an endpoint that is down only delays +//! its own deliveries, never ingest or classification. Delivered and +//! abandoned rows are kept for the retention period as the delivery log. +//! +//! Each delivery is a `POST` of a JSON body with: +//! +//! - `X-Aster-Event`: the event type, e.g. `test.quarantined` +//! - `X-Aster-Delivery`: the delivery's id, the same on every retry +//! - `X-Aster-Signature`: `sha256=` and the hex HMAC-SHA256 of the body, +//! keyed by the endpoint's secret + +use std::time::Duration; + +use anyhow::{bail, Context, Result}; +use chrono::{DateTime, Utc}; +use serde::Deserialize; +use serde_json::{json, Value}; + +use crate::db::Migration; +use crate::App; + +pub const MIGRATIONS: &[Migration] = &[Migration { + feature: "webhooks", + version: 1, + sql: r#" +CREATE SCHEMA IF NOT EXISTS webhooks; + +CREATE TABLE webhooks.deliveries ( + id bigserial PRIMARY KEY, + endpoint text NOT NULL, + event text NOT NULL, + body text NOT NULL, + created_at timestamptz NOT NULL, + attempts integer NOT NULL DEFAULT 0, + next_attempt_at timestamptz NOT NULL, + delivered_at timestamptz, + abandoned_at timestamptz, + last_status integer, + last_error text +); +CREATE INDEX deliveries_due ON webhooks.deliveries (next_attempt_at) + WHERE delivered_at IS NULL AND abandoned_at IS NULL; +CREATE INDEX deliveries_created ON webhooks.deliveries (created_at); +"#, +}]; + +/// The event types an endpoint can subscribe to. +pub const EVENTS: [&str; 4] = [ + "test.quarantined", + "test.released", + "test.expired", + "test.broken", +]; + +/// How often the sender looks for due deliveries. +pub const INTERVAL: Duration = Duration::from_secs(15); + +/// Attempts before a delivery is abandoned. With the backoff below, the +/// last one is about 14.5 hours after the first. +const MAX_ATTEMPTS: i32 = 12; + +/// How long one attempt may take. +const TIMEOUT: Duration = Duration::from_secs(10); + +/// How long a claimed delivery is held before another sender may take it. +const LEASE: chrono::Duration = chrono::Duration::minutes(5); + +/// Deliveries one pass claims. +const BATCH: i64 = 50; + +/// One configured endpoint. +#[derive(Clone, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct Endpoint { + /// Names the endpoint in the delivery log; unique. + pub name: String, + pub url: String, + pub secret: String, + /// The event types it receives. + pub events: Vec, +} + +impl std::fmt::Debug for Endpoint { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("Endpoint") + .field("name", &self.name) + .field("url", &self.url) + .field("secret", &"[redacted]") + .field("events", &self.events) + .finish() + } +} + +impl Endpoint { + /// `ASTER_SRV_WEBHOOKS`: a JSON array of + /// `{"name", "url", "secret", "events": [...]}`. + pub fn parse_all(value: &str) -> Result> { + let endpoints: Vec = serde_json::from_str(value) + .context("ASTER_SRV_WEBHOOKS must be a JSON array of {name, url, secret, events}")?; + for (index, endpoint) in endpoints.iter().enumerate() { + let name = &endpoint.name; + if name.trim().is_empty() { + bail!("webhook {index} has no name"); + } + if endpoints[..index].iter().any(|other| other.name == *name) { + bail!("two webhooks are named {name}"); + } + if !allowed_url(&endpoint.url) { + bail!( + "webhook {name}'s url must be https:// (http:// only for localhost): {}", + endpoint.url + ); + } + if endpoint.secret.len() < 16 { + bail!("webhook {name}'s secret must be at least 16 characters"); + } + if endpoint.events.is_empty() { + bail!("webhook {name} subscribes to no events"); + } + if let Some(unknown) = endpoint + .events + .iter() + .find(|event| !EVENTS.contains(&event.as_str())) + { + bail!( + "webhook {name} names an unknown event {unknown}; known: {}", + EVENTS.join(", ") + ); + } + } + Ok(endpoints) + } + + fn wants(&self, event: &str) -> bool { + self.events.iter().any(|wanted| wanted == event) + } +} + +fn allowed_url(url: &str) -> bool { + if let Some(rest) = url.strip_prefix("https://") { + return !rest.is_empty(); + } + let Some(rest) = url.strip_prefix("http://") else { + return false; + }; + let authority = rest.split('/').next().unwrap_or(""); + let host = match authority.strip_prefix('[') { + Some(v6) => v6.split(']').next().unwrap_or(""), + None => authority.split(':').next().unwrap_or(""), + }; + matches!(host, "localhost" | "127.0.0.1" | "::1") +} + +/// A notification to send to every endpoint subscribed to `event`. +#[derive(Debug, Clone, PartialEq)] +pub struct Notification { + pub event: &'static str, + pub body: Value, +} + +/// Record `notifications` for the endpoints that want them, in the caller's +/// transaction. Returns the deliveries recorded. +pub async fn enqueue( + transaction: &tokio_postgres::Transaction<'_>, + endpoints: &[Endpoint], + notifications: &[Notification], + now: DateTime, +) -> Result { + let mut recorded = 0; + for notification in notifications { + let body = notification.body.to_string(); + for endpoint in endpoints.iter().filter(|e| e.wants(notification.event)) { + recorded += transaction + .execute( + "INSERT INTO webhooks.deliveries \ + (endpoint, event, body, created_at, next_attempt_at) \ + VALUES ($1, $2, $3, $4, $4)", + &[&endpoint.name, ¬ification.event, &body, &now], + ) + .await?; + } + } + Ok(recorded) +} + +/// `sha256=` and the hex HMAC-SHA256 of `body` under `secret`. +pub fn signature(secret: &str, body: &[u8]) -> String { + use aws_lc_rs::hmac; + let key = hmac::Key::new(hmac::HMAC_SHA256, secret.as_bytes()); + let tag = hmac::sign(&key, body); + let hex: String = tag.as_ref().iter().map(|b| format!("{b:02x}")).collect(); + format!("sha256={hex}") +} + +/// The wait before attempt `attempts + 1`: 30 seconds, doubling, at most +/// six hours. +fn backoff(attempts: i32) -> chrono::Duration { + let doublings = u32::try_from(attempts.saturating_sub(1)) + .unwrap_or(0) + .min(16); + let seconds = (30i64 << doublings).min(6 * 60 * 60); + chrono::Duration::seconds(seconds) +} + +/// What a sending pass did. +#[derive(Debug, Default, PartialEq, Eq)] +pub struct Report { + pub delivered: u64, + /// Failed attempts that will be tried again. + pub retrying: u64, + pub abandoned: u64, +} + +/// Send the deliveries due at `now`. +pub async fn deliver_once(app: &App, now: DateTime) -> Result { + let client = app.pool.get().await?; + let lease = now + LEASE; + let claimed = client + .query( + "UPDATE webhooks.deliveries SET attempts = attempts + 1, next_attempt_at = $2 \ + WHERE id IN (SELECT id FROM webhooks.deliveries \ + WHERE delivered_at IS NULL AND abandoned_at IS NULL \ + AND next_attempt_at <= $1 \ + ORDER BY next_attempt_at, id LIMIT $3 \ + FOR UPDATE SKIP LOCKED) \ + RETURNING id, endpoint, event, body, attempts", + &[&now, &lease, &BATCH], + ) + .await?; + let http = crate::http_client() + .timeout(TIMEOUT) + .build() + .context("cannot build the webhook client")?; + let mut sends = tokio::task::JoinSet::new(); + for row in claimed { + let id: i64 = row.get("id"); + let name: String = row.get("endpoint"); + let event: String = row.get("event"); + let body: String = row.get("body"); + let attempts: i32 = row.get("attempts"); + let endpoint = app.config.webhooks.iter().find(|e| e.name == name).cloned(); + let http = http.clone(); + sends.spawn(async move { + let outcome = match endpoint { + None => Err(( + None, + "the endpoint is no longer configured".to_string(), + true, + )), + Some(endpoint) => send(&http, &endpoint, id, &event, body) + .await + .map_err(|(status, error)| (status, error, false)), + }; + (id, attempts, outcome) + }); + } + let mut report = Report::default(); + while let Some(joined) = sends.join_next().await { + let (id, attempts, outcome) = joined.context("a webhook send panicked")?; + match outcome { + Ok(status) => { + client + .execute( + "UPDATE webhooks.deliveries SET delivered_at = $2, last_status = $3, \ + last_error = NULL WHERE id = $1", + &[&id, &now, &status], + ) + .await?; + report.delivered += 1; + } + Err((status, error, give_up)) => { + let give_up = give_up || attempts >= MAX_ATTEMPTS; + let next = now + backoff(attempts); + client + .execute( + "UPDATE webhooks.deliveries SET last_status = $2, last_error = $3, \ + next_attempt_at = $4, \ + abandoned_at = CASE WHEN $5::boolean THEN $6::timestamptz END \ + WHERE id = $1", + &[&id, &status, &error, &next, &give_up, &now], + ) + .await?; + if give_up { + report.abandoned += 1; + } else { + report.retrying += 1; + } + } + } + } + Ok(report) +} + +/// One attempt: the response status on a 2xx, else the status (if any) and +/// why it failed. +async fn send( + http: &reqwest::Client, + endpoint: &Endpoint, + id: i64, + event: &str, + body: String, +) -> std::result::Result, String)> { + let signature = signature(&endpoint.secret, body.as_bytes()); + let response = http + .post(&endpoint.url) + .header("Content-Type", "application/json") + .header("X-Aster-Event", event) + .header("X-Aster-Delivery", id.to_string()) + .header("X-Aster-Signature", signature) + .body(body) + .send() + .await + .map_err(|error| (None, format!("{error:#}")))?; + let status = i32::from(response.status().as_u16()); + if response.status().is_success() { + Ok(status) + } else { + Err((Some(status), format!("the endpoint answered {status}"))) + } +} + +/// Send due deliveries every [`INTERVAL`], when any endpoint is configured. +pub fn spawn(app: std::sync::Arc) { + if app.config.webhooks.is_empty() { + return; + } + tokio::spawn(async move { + loop { + match deliver_once(&app, Utc::now()).await { + Ok(report) if report == Report::default() => {} + Ok(report) => crate::log::info( + "webhook deliveries", + json!({ + "delivered": report.delivered, + "retrying": report.retrying, + "abandoned": report.abandoned, + }), + ), + Err(error) => crate::log::error( + "webhook deliveries failed", + json!({"error": format!("{error:#}")}), + ), + } + tokio::time::sleep(INTERVAL).await; + } + }); +} + +#[cfg(test)] +mod tests { + use super::*; + + const ONE: &str = r#"[{"name": "relay", "url": "https://hooks.example/aster", + "secret": "0123456789abcdef", "events": ["test.quarantined", "test.broken"]}]"#; + + #[test] + fn reads_endpoints_without_printing_secrets() { + let endpoints = Endpoint::parse_all(ONE).unwrap(); + assert_eq!(endpoints.len(), 1); + assert!(endpoints[0].wants("test.broken")); + assert!(!endpoints[0].wants("test.released")); + assert!(!format!("{endpoints:?}").contains("0123456789abcdef")); + } + + #[test] + fn refuses_bad_endpoints() { + let with = |field: &str, value: &str| ONE.replace(field, value); + for bad in [ + "{}".to_string(), + with("https://hooks.example/aster", "http://hooks.example/aster"), + with("https://hooks.example/aster", "ftp://x"), + with("0123456789abcdef", "short"), + with("\"test.broken\"", "\"test.flaky\""), + with(r#"["test.quarantined", "test.broken"]"#, "[]"), + with("\"relay\"", "\" \""), + ONE.replace("]", "") + ", " + &ONE[1..], + with("\"events\"", "\"extra\": 1, \"events\""), + ] { + assert!(Endpoint::parse_all(&bad).is_err(), "{bad}"); + } + let twice = format!("[{0}, {0}]", &ONE[1..ONE.len() - 1]); + assert!(Endpoint::parse_all(&twice).is_err()); + } + + #[test] + fn the_documented_configuration_works() { + let readme = include_str!("../README.md"); + let example = readme + .split("```json\n") + .skip(1) + .filter_map(|block| block.split("```").next()) + .find(|block| block.contains("\"secret\"")) + .expect("the README shows ASTER_SRV_WEBHOOKS"); + let endpoints = Endpoint::parse_all(example).unwrap(); + assert!(endpoints[0].wants("test.quarantined")); + // Every event the README's table names is one the server sends. + for event in EVENTS { + assert!(readme.contains(&format!("| `{event}` |")), "{event}"); + } + } + + #[test] + fn plain_http_is_only_for_this_machine() { + assert!(allowed_url("http://localhost:9000/hook")); + assert!(allowed_url("http://127.0.0.1/hook")); + assert!(allowed_url("http://[::1]:80/")); + assert!(!allowed_url("http://localhost.evil.io/")); + assert!(!allowed_url("http://10.0.0.1/")); + assert!(allowed_url("https://10.0.0.1/")); + assert!(!allowed_url("https://")); + } + + #[test] + fn signs_like_the_hmac_test_vectors() { + // RFC 4231 test case 2. + assert_eq!( + signature("Jefe", b"what do ya want for nothing?"), + "sha256=5bdcc146bf60754e6a042426089575c75a003f089d2739839dec58b964ec3843" + ); + } + + #[test] + fn backs_off_by_doubling_up_to_six_hours() { + assert_eq!(backoff(1).num_seconds(), 30); + assert_eq!(backoff(2).num_seconds(), 60); + assert_eq!(backoff(5).num_seconds(), 480); + assert_eq!(backoff(10).num_seconds(), 30 << 9); + assert_eq!(backoff(11).num_seconds(), 6 * 60 * 60); + let waits: i64 = (1..MAX_ATTEMPTS).map(|a| backoff(a).num_seconds()).sum(); + assert_eq!(waits, 52_290); + assert_eq!(backoff(1000).num_seconds(), 6 * 60 * 60); + } +} diff --git a/crates/aster-srv/tests/quarantine.rs b/crates/aster-srv/tests/quarantine.rs new file mode 100644 index 0000000..d59f82b --- /dev/null +++ b/crates/aster-srv/tests/quarantine.rs @@ -0,0 +1,564 @@ +//! Quarantine and webhook notifications against a real Postgres: runs are +//! seeded at chosen times, the classifier runs with a chosen clock, and the +//! feed and the deliveries are read back. + +mod common; + +use std::sync::{Arc, Mutex}; + +use aster_srv::test_results::classifier; +use aster_srv::webhooks; +use chrono::{DateTime, Duration, Utc}; +use common::{seed, start, Run, Server, REPO}; +use serde_json::Value; + +fn ago(now: DateTime, minutes: i64) -> DateTime { + now - Duration::minutes(minutes) +} + +async fn classify(server: &Server, now: DateTime) -> classifier::Report { + classifier::run_once(&server.app, now) + .await + .unwrap() + .expect("the lock is free") +} + +/// A run in which `test` failed and then passed on its retry: one definite +/// flake event. +fn retried<'a>(run: &'a str, test: &'a str, at: DateTime) -> Run<'a> { + Run::new(run, "pr-1", at) + .attempt(test, "fail", 1, Some("f1")) + .attempt(test, "pass", 2, None) +} + +async fn feed(server: &Server) -> Value { + let (status, body) = server.get("/v1/test-results/quarantine", Some(REPO)).await; + assert_eq!(status, 200, "{body}"); + body +} + +fn quarantined(feed: &Value) -> Vec { + feed["tests"] + .as_array() + .unwrap() + .iter() + .map(|t| t["test_id"].as_str().unwrap().to_string()) + .collect() +} + +/// `(test, action)` of every log row, oldest first. +async fn log(server: &Server) -> Vec<(String, String)> { + server + .client() + .await + .query( + "SELECT test_id, action FROM test_results.quarantine_log ORDER BY id", + &[], + ) + .await + .unwrap() + .iter() + .map(|r| (r.get(0), r.get(1))) + .collect() +} + +fn entry(test: &str, action: &str) -> (String, String) { + (test.into(), action.into()) +} + +#[tokio::test] +async fn definite_flakes_in_two_runs_quarantine_a_test() { + let Some(server) = start(&[]).await else { + return; + }; + let now = Utc::now(); + seed(&server, &retried("r1", "twice", ago(now, 60))).await; + seed(&server, &retried("r2", "twice", ago(now, 50))).await; + // Two definite events, but from one run. + seed( + &server, + &Run::new("r3", "pr-2", ago(now, 45)).result("once", "pass", None), + ) + .await; + seed( + &server, + &Run::new("r4", "pr-2", ago(now, 40)) + .attempt("once", "fail", 1, Some("f1")) + .attempt("once", "pass", 2, None), + ) + .await; + let empty = feed(&server).await; + assert_eq!(empty["version"], 0); + assert!(quarantined(&empty).is_empty()); + + let report = classify(&server, now).await; + assert_eq!(report.quarantine.quarantined, 1); + + let list = feed(&server).await; + assert_eq!(quarantined(&list), vec!["twice"]); + assert_eq!(list["repo"], REPO); + assert_eq!(list["truncated"], false); + let test = &list["tests"][0]; + assert_eq!(test["target"], "//a:test"); + assert_eq!(test["file"], "test/twice_test.exs"); + assert_eq!(test["runs"], 2); + let since: DateTime = test["since"].as_str().unwrap().parse().unwrap(); + let expires: DateTime = test["expires_at"].as_str().unwrap().parse().unwrap(); + assert_eq!((expires - since).num_days(), 30); + assert!(list["version"].as_i64().unwrap() > 0); + assert_eq!(log(&server).await, vec![entry("twice", "quarantined")]); + + // The list filter shows the same tests. + let (_, page) = server + .get("/v1/test-results/tests?state=quarantined", Some(REPO)) + .await; + let listed: Vec<&str> = page["tests"] + .as_array() + .unwrap() + .iter() + .map(|t| t["test_id"].as_str().unwrap()) + .collect(); + assert_eq!(listed, vec!["twice"]); + + // Running again changes nothing and keeps the version. + let again = classify(&server, now + Duration::minutes(5)).await; + assert_eq!(again.quarantine, Default::default()); + assert_eq!(feed(&server).await["version"], list["version"]); + server.stop().await; +} + +#[tokio::test] +async fn events_older_than_seven_days_do_not_count() { + let Some(server) = start(&[]).await else { + return; + }; + let now = Utc::now(); + seed(&server, &retried("r1", "slow", ago(now, 8 * 24 * 60))).await; + seed(&server, &retried("r2", "slow", ago(now, 10))).await; + classify(&server, now).await; + assert!(quarantined(&feed(&server).await).is_empty()); + server.stop().await; +} + +#[tokio::test] +async fn the_entry_threshold_is_configurable_and_zero_turns_it_off() { + let Some(server) = start(&[("ASTER_SRV_QUARANTINE_RUNS", "1")]).await else { + return; + }; + let now = Utc::now(); + seed(&server, &retried("r1", "one", ago(now, 10))).await; + classify(&server, now).await; + assert_eq!(quarantined(&feed(&server).await), vec!["one"]); + server.stop().await; + + let Some(server) = start(&[("ASTER_SRV_QUARANTINE_RUNS", "0")]).await else { + return; + }; + seed(&server, &retried("r1", "one", ago(now, 20))).await; + seed(&server, &retried("r2", "one", ago(now, 10))).await; + classify(&server, now).await; + assert!(quarantined(&feed(&server).await).is_empty()); + server.stop().await; +} + +/// Three default-branch failures with one hash after `minutes` ago: the +/// test is broken. +async fn break_on_main(server: &Server, test: &str, now: DateTime, minutes: i64) { + for (index, run) in ["m1", "m2", "m3"].into_iter().enumerate() { + seed( + server, + &Run::new(run, "main", ago(now, minutes - index as i64)) + .hash(run) + .result(test, "fail", Some("same")), + ) + .await; + } +} + +#[tokio::test] +async fn broken_tests_are_never_quarantined_and_leave_when_they_break() { + let Some(server) = start(&[]).await else { + return; + }; + let now = Utc::now(); + // Proven flaky, and also broken on main by now. + seed(&server, &retried("r1", "both", ago(now, 60))).await; + seed(&server, &retried("r2", "both", ago(now, 50))).await; + break_on_main(&server, "both", now, 30).await; + classify(&server, now).await; + assert!(quarantined(&feed(&server).await).is_empty()); + server.stop().await; + + // Quarantined first, broken later. + let Some(server) = start(&[]).await else { + return; + }; + seed(&server, &retried("r1", "later", ago(now, 60))).await; + seed(&server, &retried("r2", "later", ago(now, 50))).await; + classify(&server, ago(now, 40)).await; + assert_eq!(quarantined(&feed(&server).await), vec!["later"]); + break_on_main(&server, "later", now, 30).await; + let report = classify(&server, now).await; + assert_eq!(report.quarantine.broken, 1); + assert!(quarantined(&feed(&server).await).is_empty()); + assert_eq!( + log(&server).await, + vec![entry("later", "quarantined"), entry("later", "broken")] + ); + server.stop().await; +} + +#[tokio::test] +async fn consecutive_passes_release_and_a_release_starts_afresh() { + let Some(server) = start(&[("ASTER_SRV_QUARANTINE_RELEASE_PASSES", "3")]).await else { + return; + }; + let now = Utc::now(); + seed(&server, &retried("r1", "t", ago(now, 100))).await; + seed(&server, &retried("r2", "t", ago(now, 90))).await; + classify(&server, ago(now, 80)).await; + assert_eq!(quarantined(&feed(&server).await), vec!["t"]); + + // pass, pass, fail, pass, pass: the fail resets the count. + for (run, minutes, outcome) in [ + ("p1", 70, "pass"), + ("p2", 65, "pass"), + ("p3", 60, "fail"), + ("p4", 55, "pass"), + ("p5", 50, "pass"), + ] { + let failure = (outcome == "fail").then_some("f9"); + seed( + &server, + &Run::new(run, "pr-9", ago(now, minutes)) + .hash(run) + .result("t", outcome, failure), + ) + .await; + } + classify(&server, ago(now, 45)).await; + assert_eq!(quarantined(&feed(&server).await), vec!["t"]); + + seed( + &server, + &Run::new("p6", "pr-9", ago(now, 40)) + .hash("p6") + .result("t", "pass", None), + ) + .await; + let report = classify(&server, ago(now, 35)).await; + assert_eq!(report.quarantine.released, 1); + assert!(quarantined(&feed(&server).await).is_empty()); + + // The events that quarantined it are still within 7 days, but only + // events after the release count. + classify(&server, ago(now, 30)).await; + assert!(quarantined(&feed(&server).await).is_empty()); + seed(&server, &retried("r3", "t", ago(now, 20))).await; + seed(&server, &retried("r4", "t", ago(now, 10))).await; + classify(&server, now).await; + assert_eq!(quarantined(&feed(&server).await), vec!["t"]); + assert_eq!( + log(&server).await, + vec![ + entry("t", "quarantined"), + entry("t", "released"), + entry("t", "quarantined") + ] + ); + server.stop().await; +} + +#[tokio::test] +async fn replayed_passes_from_cached_targets_do_not_release() { + let Some(server) = start(&[("ASTER_SRV_QUARANTINE_RELEASE_PASSES", "2")]).await else { + return; + }; + let now = Utc::now(); + seed(&server, &retried("r1", "t", ago(now, 100))).await; + seed(&server, &retried("r2", "t", ago(now, 90))).await; + classify(&server, ago(now, 80)).await; + assert_eq!(quarantined(&feed(&server).await), vec!["t"]); + + for (run, minutes) in [("c1", 70), ("c2", 65)] { + let mut replayed = Run::new(run, "pr-9", ago(now, minutes)).result("t", "pass", None); + replayed.cached = true; + seed(&server, &replayed).await; + } + assert_eq!(classify(&server, ago(now, 60)).await.quarantine.released, 0); + assert_eq!(quarantined(&feed(&server).await), vec!["t"]); + + for (run, minutes) in [("p1", 55), ("p2", 50)] { + seed( + &server, + &Run::new(run, "pr-9", ago(now, minutes)).result("t", "pass", None), + ) + .await; + } + assert_eq!(classify(&server, ago(now, 45)).await.quarantine.released, 1); + server.stop().await; +} + +#[tokio::test] +async fn entries_expire() { + let Some(server) = start(&[("ASTER_SRV_QUARANTINE_DAYS", "1")]).await else { + return; + }; + let now = Utc::now(); + seed(&server, &retried("r1", "t", ago(now, 30))).await; + seed(&server, &retried("r2", "t", ago(now, 20))).await; + classify(&server, ago(now, 10)).await; + assert_eq!(quarantined(&feed(&server).await), vec!["t"]); + let report = classify(&server, now + Duration::hours(24)).await; + assert_eq!(report.quarantine.expired, 1); + assert!(quarantined(&feed(&server).await).is_empty()); + server.stop().await; +} + +#[tokio::test] +async fn a_repository_reads_only_its_own_list() { + let Some(server) = start(&[]).await else { + return; + }; + let now = Utc::now(); + seed(&server, &retried("r1", "t", ago(now, 20))).await; + seed(&server, &retried("r2", "t", ago(now, 10))).await; + classify(&server, now).await; + let (status, other) = server + .get("/v1/test-results/quarantine", Some("ArchAstro/other")) + .await; + assert_eq!(status, 200); + assert!(quarantined(&other).is_empty()); + assert_eq!(other["version"], 0); + let (status, _) = server + .get( + "/v1/test-results/quarantine?repo=ArchAstro%2Ffirstlanding", + Some("ArchAstro/other"), + ) + .await; + assert_eq!(status, 403); + let (status, _) = server.get("/v1/test-results/quarantine", None).await; + assert_eq!(status, 401); + let (status, _) = server + .get("/v1/test-results/quarantine?limit=5", Some(REPO)) + .await; + assert_eq!(status, 400); + server.stop().await; +} + +/// A webhook receiver on this machine: every request's headers and body, +/// answered with `status`. +struct Receiver { + url: String, + received: Arc>>, +} + +async fn receiver(status: u16) -> Receiver { + use http_body_util::BodyExt; + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let url = format!("http://{}/hook", listener.local_addr().unwrap()); + let received = Arc::new(Mutex::new(Vec::new())); + let store = Arc::clone(&received); + tokio::spawn(async move { + loop { + let (stream, _) = listener.accept().await.unwrap(); + let store = Arc::clone(&store); + tokio::spawn(async move { + let service = hyper::service::service_fn( + move |request: hyper::Request| { + let store = Arc::clone(&store); + async move { + let headers = request.headers().clone(); + let body = request.into_body().collect().await.unwrap().to_bytes(); + store + .lock() + .unwrap() + .push((headers, String::from_utf8(body.to_vec()).unwrap())); + let mut response = hyper::Response::new(http_body_util::Full::new( + bytes::Bytes::new(), + )); + *response.status_mut() = hyper::StatusCode::from_u16(status).unwrap(); + Ok::<_, std::convert::Infallible>(response) + } + }, + ); + let _ = hyper::server::conn::http1::Builder::new() + .serve_connection(hyper_util::rt::TokioIo::new(stream), service) + .await; + }); + } + }); + Receiver { url, received } +} + +const SECRET: &str = "a-test-secret-0123456789"; + +fn webhooks_env(url: &str, events: &str) -> String { + format!(r#"[{{"name": "relay", "url": "{url}", "secret": "{SECRET}", "events": [{events}]}}]"#) +} + +async fn deliveries(server: &Server) -> Vec<(String, i32, bool, bool)> { + server + .client() + .await + .query( + "SELECT event, attempts, delivered_at IS NOT NULL, abandoned_at IS NOT NULL \ + FROM webhooks.deliveries ORDER BY id", + &[], + ) + .await + .unwrap() + .iter() + .map(|r| (r.get(0), r.get(1), r.get(2), r.get(3))) + .collect() +} + +#[tokio::test] +async fn subscribed_endpoints_get_signed_notifications() { + let hook = receiver(204).await; + let config = webhooks_env(&hook.url, r#""test.quarantined", "test.broken""#); + let Some(server) = start(&[("ASTER_SRV_WEBHOOKS", config.as_str())]).await else { + return; + }; + let now = Utc::now(); + seed(&server, &retried("r1", "flaky", ago(now, 60))).await; + seed(&server, &retried("r2", "flaky", ago(now, 50))).await; + break_on_main(&server, "gone", now, 40).await; + let report = classify(&server, now).await; + assert_eq!(report.deliveries, 2); + // Nothing is sent until the sender runs. + assert!(hook.received.lock().unwrap().is_empty()); + + let sent = webhooks::deliver_once(&server.app, now).await.unwrap(); + assert_eq!(sent.delivered, 2); + let received = hook.received.lock().unwrap().clone(); + let mut events: Vec<(String, Value)> = received + .iter() + .map(|(headers, body)| { + let event = headers["x-aster-event"].to_str().unwrap().to_string(); + assert_eq!( + headers["x-aster-signature"].to_str().unwrap(), + webhooks::signature(SECRET, body.as_bytes()) + ); + assert!(headers.contains_key("x-aster-delivery")); + (event, serde_json::from_str(body).unwrap()) + }) + .collect(); + events.sort_by(|a, b| a.0.cmp(&b.0)); + assert_eq!(events[0].0, "test.broken"); + assert_eq!(events[0].1["test"]["test_id"], "gone"); + assert_eq!(events[0].1["detail"]["since_sha"], "m1"); + assert_eq!(events[0].1["repo"], REPO); + assert_eq!(events[1].0, "test.quarantined"); + assert_eq!(events[1].1["event"], "test.quarantined"); + assert_eq!(events[1].1["test"]["name"], "flaky"); + assert_eq!(events[1].1["detail"]["runs"], 2); + + // Delivered rows are not sent again. + let again = webhooks::deliver_once(&server.app, now + Duration::hours(1)) + .await + .unwrap(); + assert_eq!(again, Default::default()); + assert_eq!(hook.received.lock().unwrap().len(), 2); + server.stop().await; +} + +#[tokio::test] +async fn a_failing_endpoint_is_retried_with_backoff_and_blocks_nothing() { + let hook = receiver(500).await; + let config = webhooks_env(&hook.url, r#""test.quarantined""#); + let Some(server) = start(&[("ASTER_SRV_WEBHOOKS", config.as_str())]).await else { + return; + }; + let now = Utc::now(); + seed(&server, &retried("r1", "t", ago(now, 60))).await; + seed(&server, &retried("r2", "t", ago(now, 50))).await; + classify(&server, now).await; + assert_eq!(quarantined(&feed(&server).await), vec!["t"]); + + let first = webhooks::deliver_once(&server.app, now).await.unwrap(); + assert_eq!(first.retrying, 1); + // Not due again until the backoff has passed. + let early = webhooks::deliver_once(&server.app, now + Duration::seconds(10)) + .await + .unwrap(); + assert_eq!(early, Default::default()); + let second = webhooks::deliver_once(&server.app, now + Duration::seconds(31)) + .await + .unwrap(); + assert_eq!(second.retrying, 1); + assert_eq!( + deliveries(&server).await, + vec![("test.quarantined".into(), 2, false, false)] + ); + let row = server + .client() + .await + .query_one( + "SELECT last_status, last_error FROM webhooks.deliveries", + &[], + ) + .await + .unwrap(); + assert_eq!(row.get::<_, Option>(0), Some(500)); + assert!(row.get::<_, String>(1).contains("500")); + assert_eq!(hook.received.lock().unwrap().len(), 2); + + // After the last attempt the delivery is abandoned. + let mut at = now; + for _ in 0..20 { + at += Duration::hours(7); + webhooks::deliver_once(&server.app, at).await.unwrap(); + } + assert_eq!( + deliveries(&server).await, + vec![("test.quarantined".into(), 12, false, true)] + ); + server.stop().await; +} + +#[tokio::test] +async fn unsubscribed_or_unconfigured_events_record_nothing() { + let hook = receiver(204).await; + let config = webhooks_env(&hook.url, r#""test.released""#); + let Some(server) = start(&[("ASTER_SRV_WEBHOOKS", config.as_str())]).await else { + return; + }; + let now = Utc::now(); + seed(&server, &retried("r1", "t", ago(now, 60))).await; + seed(&server, &retried("r2", "t", ago(now, 50))).await; + assert_eq!(classify(&server, now).await.deliveries, 0); + server.stop().await; + + let Some(server) = start(&[]).await else { + return; + }; + seed(&server, &retried("r1", "t", ago(now, 60))).await; + seed(&server, &retried("r2", "t", ago(now, 50))).await; + assert_eq!(classify(&server, now).await.deliveries, 0); + assert!(deliveries(&server).await.is_empty()); + server.stop().await; +} + +#[tokio::test] +async fn a_pass_behind_the_committed_watermark_changes_and_announces_nothing() { + let hook = receiver(204).await; + let config = webhooks_env(&hook.url, r#""test.broken""#); + let Some(server) = start(&[("ASTER_SRV_WEBHOOKS", config.as_str())]).await else { + return; + }; + let now = Utc::now(); + break_on_main(&server, "gone", now, 30).await; + assert_eq!(classify(&server, now).await.deliveries, 1); + // A replica whose clock is behind: as of 29 minutes ago the test was + // not broken yet. Re-deriving that would undo the state and announce + // the break a second time on the next pass. + assert_eq!(classify(&server, ago(now, 29)).await, Default::default()); + let next = classify(&server, now + Duration::minutes(5)).await; + assert_eq!((next.tests_updated, next.deliveries), (0, 0)); + assert_eq!( + deliveries(&server).await, + vec![("test.broken".into(), 0, false, false)] + ); + server.stop().await; +} diff --git a/crates/aster-srv/tests/server.rs b/crates/aster-srv/tests/server.rs index c44b60e..8c73351 100644 --- a/crates/aster-srv/tests/server.rs +++ b/crates/aster-srv/tests/server.rs @@ -166,6 +166,7 @@ fn stream(run_id: &str, repo: &str, tests: usize) -> Vec { duration_ms: Some(5), failure_hash: failed.then(|| "f3a9".into()), message: failed.then(|| "** (DBConnection.OwnershipError) boom".into()), + quarantined: false, })); } records diff --git a/crates/aster-srv/tests/ui.rs b/crates/aster-srv/tests/ui.rs index 0d7e9f1..804758f 100644 --- a/crates/aster-srv/tests/ui.rs +++ b/crates/aster-srv/tests/ui.rs @@ -183,13 +183,44 @@ async fn clusters_group_recent_failures_by_hash() { } #[tokio::test] -async fn the_quarantine_page_says_it_is_not_enabled() { +async fn the_quarantine_page_lists_quarantined_tests() { let Some(server) = seeded().await else { return; }; - let (status, body) = page(&server, "/ui/quarantine?repo=ArchAstro%2Ffirstlanding").await; + let path = "/ui/quarantine?repo=ArchAstro%2Ffirstlanding"; + let (status, body) = page(&server, path).await; assert_eq!(status, 200); - assert!(body.contains("Quarantine is not enabled yet")); + assert!(body.contains("No tests are quarantined")); + + let flaky: String = server + .client() + .await + .query_one( + "SELECT test_id FROM test_results.tests WHERE state = 'flaky' LIMIT 1", + &[], + ) + .await + .unwrap() + .get(0); + server + .client() + .await + .batch_execute(&format!( + "INSERT INTO test_results.quarantine (repo, test_id, entered_at, expires_at, runs) \ + VALUES ('{REPO}', '{flaky}', now(), now() + interval '30 days', 2); \ + INSERT INTO test_results.quarantine_log (repo, test_id, action, at, detail) \ + VALUES ('{REPO}', '{flaky}', 'quarantined', now(), '{{}}')" + )) + .await + .unwrap(); + let (status, body) = page(&server, path).await; + assert_eq!(status, 200); + assert!(!body.contains("No tests are quarantined")); + assert!( + body.contains("/ui/tests/flaky%20%3Ctest%3E?repo="), + "{body}" + ); + assert!(body.contains("Recent changes")); server.stop().await; } diff --git a/docs/specs/test-tracking.html b/docs/specs/test-tracking.html index 7b22b9b..abb912a 100644 --- a/docs/specs/test-tracking.html +++ b/docs/specs/test-tracking.html @@ -309,7 +309,7 @@

Aster Test Tracking

RetentionA daily job drops partitions older than 30 days. A GCS lifecycle rule deletes raw objects at 30 days. tests rows not seen for 30 days are deleted. DeployImage ghcr.io/archastro/aster-srv from this repo's release workflow. Pulumi stack in firstlanding/infra/services/aster-srv: Deployment, Cloud SQL, bucket, Workload Identity. NotificationsConfigured webhook endpoints, each with its own subscribed event types: test.quarantined, test.released, test.expired, test.broken. The JSON payload is signed with HMAC-SHA256 (X-Aster-Signature) and retried with backoff. Slack, issue trackers and other receivers live outside the service. - RoutesPOST /v1/test-results/runs · GET /v1/test-results/quarantine?repo= · GET /v1/test-results/tests?state=flaky|broken|quarantined · GET /v1/test-results/tests/:id · GET /v1/test-results/runs/:run_id/failures · GET /v1/test-results/durations?target=&day= · /healthz + RoutesPOST /v1/test-results/runs · GET /v1/test-results/quarantine · GET /v1/test-results/tests?state=flaky|broken|quarantined · GET /v1/test-results/tests/:id · GET /v1/test-results/runs/:run_id/failures · GET /v1/test-results/durations?target=&day= · /healthz @@ -341,7 +341,7 @@

Aster Test Tracking

Open questions
    -
  1. Auto-quarantine threshold: should a test enter quarantine after 2 proven flakes from 2 different runs in 7 days (the current proposal), or after a single one? A proven flake means the test passed and failed on identical inputs (the same input_hash). One event is proof enough on its own, but an outage across CI (database down, network blip) would then quarantine every test it touched in a single run.
    2. +
  2. Auto-quarantine threshold (built as the proposal, and configurable: ASTER_SRV_QUARANTINE_RUNS, default 2; 1 means a single proven flake): should a test enter quarantine after 2 proven flakes from 2 different runs in 7 days, or after a single one? A proven flake means the test passed and failed on identical inputs (the same input_hash). One event is proof enough on its own, but an outage across CI (database down, network blip) would then quarantine every test it touched in a single run.
@@ -443,15 +443,15 @@

Aster Test Tracking

files: ["crates/aster-srv/src/ui/"], deps: ["T17"] }, { id: "T22", p: "P4", area: "service", title: "Quarantine store + feed", sub: "Auto entry and release", - obj: "Automatically quarantine at 2 definite events from 2 or more runs in 7 days. Release after 20 consecutive passes, or expire at 30 days. GET /v1/test-results/quarantine returns a versioned list.", + obj: "At the end of each classifier pass: quarantine a test that is not broken once definite flakes from 2 different runs fall in 7 days (ASTER_SRV_QUARANTINE_RUNS; 0 turns it off). Release after 20 consecutive passes (ASTER_SRV_QUARANTINE_RELEASE_PASSES), when it breaks, or at 30 days (ASTER_SRV_QUARANTINE_DAYS); a released test is judged only on later events. GET /v1/test-results/quarantine returns the list, versioned by its newest log id; tests?state=quarantined filters by it; /ui/quarantine shows it with recent changes.", acc: ["Broken tests never quarantined", "Every state change kept in a log table (30 days)"], - files: ["crates/aster-srv/src/quarantine.rs"], deps: ["T16"] }, + files: ["crates/aster-srv/src/test_results/quarantine.rs", "crates/aster-srv/src/test_results/classifier.rs", "crates/aster-srv/src/ui/"], deps: ["T16"] }, { id: "T23", p: "P4", area: "client", title: "Quarantine enforcement", sub: "Repo file + server list, the 4 conditions", - obj: "Fetch at start (cached for 24h in .aster/quarantine.json), merge with aster-quarantine.toml, apply the 4 suppression conditions, and report passed (quarantined: N).", + obj: "With capture on, fetch the server list at start (cached in .aster/quarantine.json; a cache under 24h stands in when the server is down), merge with aster-quarantine.toml ([[test]] by id or name/file/suite/target, each with reason and expires), apply the 4 conditions after retries, and report passed (quarantined: N). Tests-failed exit codes: ExUnit 2, nextest 100, pytest/go/jest/vitest/bun/node 1; never Gradle, Maven or cargo test. Suppressed failures get a warning annotation, a step-summary row and quarantined: true in the results file; the pass is not cached.", acc: ["Partial-confidence results never suppressed", "A crash or compile error never suppressed", "ASTER_QUARANTINE=off disables it", "Dependent targets of a quarantine-passed target run"], - files: ["src/test_results/quarantine.rs", "src/executor/runner.rs", "src/cli/output.rs"], deps: ["T22", "T15"] }, + files: ["src/test_results/quarantine.rs", "src/executor/runner.rs", "src/cli/output.rs", "src/test_results/summary.rs"], deps: ["T22", "T15"] }, { id: "T26", p: "P4", area: "service", title: "Webhook notifications", sub: "Configured endpoints, HMAC-signed", - obj: "Config sets a list of webhook endpoints, each with a URL, a secret and the event types it receives (test.quarantined, test.released, test.expired, test.broken). Deliveries are queued in Postgres and retried with exponential backoff.", + obj: "ASTER_SRV_WEBHOOKS (JSON) lists endpoints, each with a name, URL, secret and the event types it receives (test.quarantined, test.released, test.expired, test.broken). The classifier records deliveries in its own transaction (outbox); a sender posts them every 15s and retries from 30s, doubling to 6h, abandoning after 12 attempts.", acc: ["Payload signed as X-Aster-Signature: sha256=", "A failing endpoint never blocks ingest or classification", "Delivery log kept 30 days", "No endpoints configured means nothing is sent"], files: ["crates/aster-srv/src/webhooks.rs", "crates/aster-srv/src/config.rs"], deps: ["T22"] }, { id: "T24", p: "P4", area: "client", title: "Duration-balanced mix partitions", sub: "p50 file durations, longest first", @@ -459,9 +459,9 @@

Aster Test Tracking

acc: ["Every shard of one commit gets the same split: no file runs on no shard or on two", "Falls back to Mix's own split, unchanged, on every shard alike: no aster-durations.json, the suite missing from it, a malformed file, or ASTER_BALANCE_PARTITIONS=off", "Retry and result capture work on the rewritten command", "Slowest partition's time drops on firstlanding (measured after rollout; logs show up to ~20% headroom)"], files: ["src/plugins/mix_partitions.rs", "src/test_results/balance.rs", "src/executor/runner/partitions.rs", "crates/aster-srv/src/test_results/durations.rs"], deps: ["T17"] }, { id: "T25", p: "P4", area: "docs", title: "Docs", sub: "README, GETTING_STARTED, service runbook", - obj: "Document capture per runner, the env vars (ASTER_SRV_URL, ASTER_QUARANTINE), the quarantine rules and self-hosting.", + obj: "Document capture per runner, the env vars (ASTER_SRV_URL, ASTER_QUARANTINE), the quarantine rules, webhooks, self-hosting and a service runbook.", acc: ["Doc examples run against a fixture workspace"], - files: ["README.md", "docs/test-tracking.md"], deps: ["T23"] } + files: ["README.md", "GETTING_STARTED.md", "docs/test-tracking.md", "crates/aster-srv/README.md"], deps: ["T23"] } ]; // Shared status lives in the artifact db: status/ = {status, note, pr, updated_at}. diff --git a/docs/test-tracking.md b/docs/test-tracking.md new file mode 100644 index 0000000..0d58e12 --- /dev/null +++ b/docs/test-tracking.md @@ -0,0 +1,182 @@ +# Test tracking + +Aster can record every test a CI run executes, retry only the tests that +failed, tell flaky tests from broken ones across runs, and keep known flakes +from failing the build. This page covers the whole loop: + +```text +CI runner aster-srv (Postgres) +--------- -------------------- +aster --test-results --retry-failed ──▶ POST /v1/test-results/runs + capture each runner's report classifier, every 5 min: + retry failing tests flaky / broken / healthy + quarantine list at start ◀────────────── quarantine list + webhooks + step summary + annotations /ui dashboard (behind IAP) +``` + +Everything on the CI side works without a server. A server adds history, +labels (`new`, `broken`, `known flake`) and the automatic quarantine list. + +## 1. Capture + +`--test-results[=PATH]` (or `ASTER_TEST_RESULTS=PATH`, or `=1` for a temp +path) reads each runner's own report and writes one NDJSON file per Aster +invocation: a run record, a record per target, and one per test attempt. + +| Runner | How Aster reads it | Confidence | +| --- | --- | --- | +| `mix test` (ExUnit) | Aster's formatter, loaded beside `ExUnit.CLIFormatter`, checked against ExUnit's summary | complete | +| `go test` | `-json`, rendered back to plain `go test` output in the log | complete | +| `pytest` | `--junitxml` | complete | +| Vitest, `bun test` | JUnit reporter beside the default one | complete | +| Jest | `--json` | complete | +| `node --test` | Aster's reporter | complete | +| `cargo nextest run` | JUnit profile | complete | +| `cargo test` | console `test x ... ok/FAILED` lines | partial | +| Gradle, Maven | existing `build/test-results` and `target/surefire-reports` files | complete | + +`partial` means some tests may be missing. Partial results are still uploaded +and classified, but they never let a quarantined failure pass. + +## 2. Retry + +`--retry-failed[=N]` (or `ASTER_RETRY_FAILED=N`) reruns only the failing +tests, with the runner's own selection flags, and records each attempt. See +[Retrying failed tests](../README.md#retrying-failed-tests) for the runner +table and `retry_setup`. + +## 3. Upload and labels + +| Variable | | +| --- | --- | +| `ASTER_SRV_URL` | The server. Set, Aster uploads after the run and fetches the quarantine list before it. | +| `ASTER_SRV_TOKEN` | A static token. Without it, GitHub Actions' OIDC token is used (the job needs `permissions: id-token: write`). | +| `ASTER_SRV_AUDIENCE` | The OIDC audience; default `aster-srv`. | +| `ASTER_SRV_PROXY_TOKEN` | Sent as `Proxy-Authorization` when the server sits behind an identity-aware proxy. | +| `ASTER_GITHUB_SUMMARY` | `0` stops the step summary and annotations in GitHub Actions. | +| `ASTER_QUARANTINE` | `off` stops quarantine; failures fail as usual. | +| `ASTER_BALANCE_PARTITIONS` | `off` keeps Mix's own split for `mix test --partitions` shards. | + +An upload or lookup that fails never fails the build. `aster results upload +`, `aster results summary [--github]` and `aster results failures +--github-run ` do the same steps by hand. + +A GitHub Actions job: + +```yaml +permissions: + id-token: write + contents: read +steps: + - run: aster affected test --retry-failed --test-results + env: + ASTER_SRV_URL: https://aster-srv.example.com +``` + +The step summary lists each failing test as `new`, `broken` (with the commit +it broke at), `known flake`, `quarantined` or, without a server, `failed`, +and each gets an annotation on its file. + +## 4. Flaky and broken + +The server classifies every run. A test is a **definite flake** when it failed +and then passed on a retry in one run, or when two runs with the same target +`input_hash` (the same inputs) disagree. It is **broken** when its last 3 +default-branch runs failed the same way; broken tests are never quarantined. +The details are in the [server README](../crates/aster-srv/README.md#flaky-and-broken-tests). + +## 5. Quarantine + +A quarantined test still runs and still uploads. When it fails, its target +may report `passed (quarantined: N)` instead of failing, and its dependents +run. + +### What makes a failure pass + +All of these, after retries: + +1. The target's results are `complete`. +2. Every test whose last attempt failed is quarantined, and there is at least + one. +3. The command exited with its runner's "tests failed" code: ExUnit 2, + nextest 100, pytest, `go test`, Jest, Vitest, `bun test` and `node --test` + 1. A crash, compile error, timeout or signal exits otherwise and still + fails. Gradle, Maven and `cargo test` use one code for test failures and + compile errors, so their targets never pass this way. +4. `ASTER_QUARANTINE` is not `off`. + +A target that runs several commands, or whose results are not captured (no +`--test-results` and no `--retry-failed`), is never judged. When some failures +are quarantined but the target still fails, its log says why. + +Each failure let through gets a GitHub `::warning` annotation and a row in the +step summary; `--json` lists them under `tests.quarantined`; the results file +marks those attempts `"quarantined": true`. A pass that needed the quarantine +is not cached. + +### Where the list comes from + +At start Aster prints the list it will use, for example: + +```text +[aster] Quarantine: 4 from aster-srv (version 312), 1 from aster-quarantine.toml +``` + +**The server** quarantines a test that is not broken once definite flakes +from 2 different runs fall within 7 days. It releases the test after 20 +consecutive passes, when it breaks, or after 30 days; a released test starts +afresh. The list is fetched at start and cached in `.aster/quarantine.json`. +If the server cannot be reached, a cache younger than 24 hours stands in; +otherwise only the repository file applies, so nothing extra is let through. + +**The repository** can quarantine tests itself in `aster-quarantine.toml` at +the workspace root, reviewed like any change. Every entry needs a `reason` and +an `expires` date, and stops applying after that date (with a warning). An +entry names a test by `id` (shown on the dashboard), or by `name` with +optional `file`, `suite` and `target` (a project or one of its targets) to +narrow it: + +```toml +# aster-quarantine.toml +[[test]] +name = "test creates an agent" +file = "test/platform/agents_test.exs" +target = "//services/platform" +reason = "DBConnection.OwnershipError under load; tracked in #4812" +expires = 2026-11-30 + +[[test]] +id = "c81e728d9d4c2f636f067f89cc14862c0f5b6a1e9e9e0f4c6b1a2d3e4f5a6b7c" +reason = "Times out on the 4-core runners" +expires = 2026-11-15 +``` + +A malformed file stops the run with an error naming the entry. + +## 6. Balanced partitions + +A target that runs one partition of a Mix suite +(`MIX_TEST_PARTITION=K mix test --partitions N`) runs an explicit file list +when `aster-durations.json` at the workspace root times its suite: the +suite's files split longest-first by median duration. The file comes from +the checkout, so every shard of a commit splits the same way; nothing is +fetched while the suite runs. `aster results durations` writes it from the +server; commit it and refresh it on a schedule (see +[Durations](../crates/aster-srv/README.md#durations)). Without the file, the +suite in it, or with `ASTER_BALANCE_PARTITIONS=off`, every shard keeps Mix's +own split. A shard's tests are recorded under the project whose suite it +runs (the directory the command `cd`s to), so a test has one id, and one +quarantine entry, whichever shard ran it. + +## 7. Notifications + +The server can POST to webhooks when a test is quarantined, released, expires +from quarantine, or breaks. See +[Webhooks](../crates/aster-srv/README.md#webhooks). + +## 8. Self-hosting + +aster-srv is one binary (and the image +`ghcr.io/archastro/aster-srv:`) that needs Postgres. The +[server README](../crates/aster-srv/README.md) covers configuration, +authentication, the dashboard and operating it. diff --git a/src/cli/output.rs b/src/cli/output.rs index 5282490..68bd9a2 100644 --- a/src/cli/output.rs +++ b/src/cli/output.rs @@ -127,6 +127,10 @@ pub struct TestSummary { /// The tests that failed and then passed on a retry #[serde(skip_serializing_if = "Vec::is_empty")] pub flaky: Vec, + /// Failing tests the quarantine let through: the target passed + /// because of them + #[serde(skip_serializing_if = "Vec::is_empty")] + pub quarantined: Vec, } /// One test that passed only on a retry @@ -189,6 +193,7 @@ fn test_summary(report: &crate::test_results::TestReport) -> TestSummary { passed_on_attempt: test.passed_on_attempt, }) .collect(), + quarantined: report.quarantined.clone(), } } @@ -265,6 +270,23 @@ pub fn print_summary( } println!("Total time: {total_duration}ms"); + let quarantined: Vec<_> = results + .iter() + .filter_map(|r| { + let tests = &r.tests.as_ref()?.quarantined; + (r.success && !tests.is_empty()).then_some((r, tests)) + }) + .collect(); + if !quarantined.is_empty() { + println!("\nPassed with quarantined failures:"); + for (result, tests) in quarantined { + println!(" - {} (quarantined: {})", result.address, tests.len()); + for test in tests { + println!(" {} ({})", test.name, test.source); + } + } + } + if failed > 0 { println!("\nFailed projects:"); for result in results.iter().filter(|r| !r.success) { @@ -512,6 +534,7 @@ mod tests { case("c", TestOutcome::Error), case("d", TestOutcome::Skip), ], + quarantined: Vec::new(), }), input_hash: None, }; diff --git a/src/executor/runner.rs b/src/executor/runner.rs index d3f31ab..1ba55a9 100644 --- a/src/executor/runner.rs +++ b/src/executor/runner.rs @@ -31,6 +31,7 @@ use crate::discovery::DiscoveredProject; use crate::executor::logs::{LogStore, RunLog, TargetLog}; use crate::graph::ProjectGraph; use crate::plugins::{LanguagePlugin, PluginRegistry}; +use crate::test_results::quarantine::{Quarantine, Verdict}; use crate::test_results::{ self, Confidence, OutputRewrite, ReportContext, ReportFiles, ReportPlan, TestReport, }; @@ -153,6 +154,8 @@ pub struct Executor<'a> { retry_failed: u32, /// Maximum combined stdout+stderr bytes retained for one command. log_cap: usize, + /// The tests whose failures may not fail their target. + quarantine: Option>, } /// Default cap on one command's combined log: 16 MiB. @@ -171,6 +174,7 @@ impl<'a> Executor<'a> { capture_tests: false, retry_failed: 0, log_cap: DEFAULT_LOG_CAP_BYTES, + quarantine: None, } } @@ -186,6 +190,7 @@ impl<'a> Executor<'a> { capture_tests: false, retry_failed: 0, log_cap: DEFAULT_LOG_CAP_BYTES, + quarantine: None, } } @@ -205,6 +210,7 @@ impl<'a> Executor<'a> { capture_tests: false, retry_failed: 0, log_cap: DEFAULT_LOG_CAP_BYTES, + quarantine: None, } } @@ -225,6 +231,7 @@ impl<'a> Executor<'a> { capture_tests: false, retry_failed: 0, log_cap: DEFAULT_LOG_CAP_BYTES, + quarantine: None, } } @@ -265,6 +272,14 @@ impl<'a> Executor<'a> { self } + /// Let a failed test target pass when, after retries, its only + /// failures are quarantined tests (see [`crate::test_results::quarantine`]). + /// Applies only to targets whose results are captured. + pub fn with_quarantine(mut self, quarantine: Option) -> Self { + self.quarantine = quarantine.map(Arc::new); + self + } + /// Execute a target on selected projects in dependency order /// /// Respects target dependencies from Target.depends_on (fully resolved by TargetResolver). @@ -865,6 +880,12 @@ impl<'a> Executor<'a> { let retry_command = (self.retry_failed > 0 && commands.len() == 1).then(|| commands[0].clone()); let retry_failed = self.retry_failed; + // Quarantine judges one command's results, like a retry. + let quarantine = self + .quarantine + .clone() + .filter(|_| !captures.is_empty() && commands.len() == 1); + let quarantine_command = commands.first().cloned(); let retry_setup = project .targets .get(&target_name) @@ -946,6 +967,50 @@ impl<'a> Executor<'a> { } } } + let mut let_through = false; + if let (Some(quarantine), Some(command), Some(report), false) = ( + &quarantine, + &quarantine_command, + result.tests.as_ref(), + result.success || result.skipped || shutdown_requested(), + ) { + match quarantine.judge(&addr, &plugin_name, command, result.exit_code, report) { + Verdict::Pass(tests) => { + let mut note = format!( + "[aster] Quarantine: every failing test is quarantined, so {addr} passes\n" + ); + for test in &tests { + note.push_str(&format!( + "[aster] {} ({})\n", + test.name, test.source + )); + } + retry::append_log(&mut result, ¬e, &[]); + if std::env::var("GITHUB_ACTIONS").as_deref() == Ok("true") { + for line in retry::quarantine_annotations( + &tests, + &working_dir, + &workspace_root_path, + ) { + eprintln!("{line}"); + } + } + if let Some(report) = result.tests.as_mut() { + report.quarantined = tests; + } + result.success = true; + let_through = true; + } + Verdict::Fail(why) => { + retry::append_log( + &mut result, + &format!("[aster] Quarantine: {why}\n"), + &[], + ); + } + Verdict::Untouched => {} + } + } result.input_hash = input_hash; if shutdown_requested() { result.success = false; @@ -957,9 +1022,10 @@ impl<'a> Executor<'a> { }; } - // A pass that needed a retry is not a clean pass to replay - // from the cache: the next run should see the flake again. - if !result.success || retried { + // A pass that needed a retry or the quarantine is not a + // clean pass to replay from the cache: the next run should + // see the flake again. + if !result.success || retried || let_through { if let Some(cache_dir) = cache_store_path.as_ref() { let store = CacheStore::new(cache_dir); if let Err(e) = store.remove(&addr) { @@ -1069,6 +1135,10 @@ impl<'a> Executor<'a> { if flaky > 0 { progress.mark_flaky(&result.address, flaky); } + let quarantined = result.tests.as_ref().map_or(0, |t| t.quarantined.len()); + if quarantined > 0 { + progress.mark_quarantined(&result.address, quarantined); + } } progress.mark_complete( &result.address, @@ -1836,6 +1906,7 @@ fn read_test_report( Confidence::Partial }, cases, + quarantined: Vec::new(), }) } diff --git a/src/executor/runner/retry.rs b/src/executor/runner/retry.rs index 506a65d..0f0bc46 100644 --- a/src/executor/runner/retry.rs +++ b/src/executor/runner/retry.rs @@ -8,6 +8,7 @@ use super::*; use crate::plugins::test_retry; +use crate::test_results::quarantine::Quarantined; use crate::test_results::same_test; use crate::test_results::{TestCase, TestOutcome}; @@ -153,6 +154,7 @@ pub(super) fn rerun_failures(job: &RetryJob, result: &mut ExecutionResult) -> bo capture: report.capture, confidence, cases, + quarantined: Vec::new(), }); } ran_any @@ -225,7 +227,7 @@ fn remove_report_dirs(captures: &[Vec>]) { } /// Add a note and a command's log to the result's log. -fn append_log(result: &mut ExecutionResult, note: &str, log: &[u8]) { +pub(super) fn append_log(result: &mut ExecutionResult, note: &str, log: &[u8]) { if !result.log.is_empty() && !result.log.ends_with(b"\n") { result.log.push(b'\n'); } @@ -262,23 +264,57 @@ pub(super) fn flaky_annotations( .flaky() .iter() .map(|test| { - let message = escape_data(&format!("{} failed then passed on retry", test.name)); - match &test.file { - Some(file) => { - let absolute = working_dir.join(file); - let path = absolute - .strip_prefix(workspace_root) - .unwrap_or(Path::new(file)) - .to_string_lossy() - .replace('\\', "/"); - format!("::warning file={}::{message}", escape_property(&path)) - } - None => format!("::warning::{message}"), - } + warning( + test.file.as_deref(), + &format!("{} failed then passed on retry", test.name), + working_dir, + workspace_root, + ) }) .collect() } +/// The GitHub Actions annotations for failures the quarantine let through: +/// one warning per test, on its file when known. +pub(super) fn quarantine_annotations( + tests: &[Quarantined], + working_dir: &Path, + workspace_root: &Path, +) -> Vec { + tests + .iter() + .map(|test| { + warning( + test.file.as_deref(), + &format!( + "{} failed but is quarantined ({}); its target passed", + test.name, test.source + ), + working_dir, + workspace_root, + ) + }) + .collect() +} + +/// A `::warning` annotation, on `file` (relative to `working_dir`) when +/// known. +fn warning(file: Option<&str>, message: &str, working_dir: &Path, workspace_root: &Path) -> String { + let message = escape_data(message); + match file { + Some(file) => { + let absolute = working_dir.join(file); + let path = absolute + .strip_prefix(workspace_root) + .unwrap_or(Path::new(file)) + .to_string_lossy() + .replace('\\', "/"); + format!("::warning file={}::{message}", escape_property(&path)) + } + None => format!("::warning::{message}"), + } +} + fn escape_data(text: &str) -> String { text.replace('%', "%25") .replace('\r', "%0D") @@ -320,6 +356,7 @@ mod tests { capture: "go-json".into(), confidence: Confidence::Complete, cases, + quarantined: Vec::new(), }), input_hash: None, } @@ -436,6 +473,7 @@ mod tests { case("TestA 100%", TestOutcome::Fail, 1), case("TestA 100%", TestOutcome::Pass, 2), ], + quarantined: Vec::new(), }; let lines = flaky_annotations(&report, Path::new("/ws/app"), Path::new("/ws")); assert_eq!( diff --git a/src/main.rs b/src/main.rs index 110f9da..24def44 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1116,7 +1116,11 @@ fn run() -> Result<()> { !cli.no_cache, ) .with_test_capture(test_results.is_some()) - .with_retry_failed(cli.retry_failed()); + .with_retry_failed(cli.retry_failed()) + .with_quarantine(quarantine_for( + &workspace_root, + test_results.is_some() || cli.retry_failed() > 0, + )?); let all_project_refs: Vec<_> = projects.iter().collect(); let results = if let Some(plan) = &command_plan { @@ -1244,7 +1248,11 @@ fn run() -> Result<()> { let executor = Executor::with_all_options(&workspace_root, output_mode, full_logs, !cli.no_cache) .with_test_capture(test_results.is_some()) - .with_retry_failed(cli.retry_failed()); + .with_retry_failed(cli.retry_failed()) + .with_quarantine(quarantine_for( + &workspace_root, + test_results.is_some() || cli.retry_failed() > 0, + )?); let results = executor.execute_targets(&primary_targets, &all_project_refs, no_deps); write_test_results(test_results, run_record, &results); @@ -1544,7 +1552,11 @@ fn run() -> Result<()> { let executor = Executor::with_all_options(&workspace_root, output_mode, full_logs, !cli.no_cache) .with_test_capture(test_results.is_some()) - .with_retry_failed(cli.retry_failed()); + .with_retry_failed(cli.retry_failed()) + .with_quarantine(quarantine_for( + &workspace_root, + test_results.is_some() || cli.retry_failed() > 0, + )?); // Handle streaming execution (single project only, output to terminal) if should_stream { @@ -1772,6 +1784,28 @@ fn handle_results(command: &ResultsCommands, output_mode: OutputMode) -> Result< } } +/// The quarantine for a run, loaded once per invocation (a run over +/// several lanes executes several times). Only a run that captures test +/// results can use one. +fn quarantine_for( + workspace_root: &Path, + capturing: bool, +) -> Result> { + static LOADED: std::sync::OnceLock< + std::result::Result, String>, + > = std::sync::OnceLock::new(); + if !capturing { + return Ok(None); + } + LOADED + .get_or_init(|| { + aster::test_results::quarantine::load(workspace_root, &|name| env::var(name).ok()) + .map_err(|error| format!("{error:#}")) + }) + .clone() + .map_err(anyhow::Error::msg) +} + /// The server URL from `--url`, else `ASTER_SRV_URL`. fn srv_url(flag: Option<&str>) -> Option { flag.map(str::to_string) diff --git a/src/test_results/mod.rs b/src/test_results/mod.rs index ce53e22..58ac5bb 100644 --- a/src/test_results/mod.rs +++ b/src/test_results/mod.rs @@ -18,7 +18,8 @@ mod jest; mod junit; mod libtest; pub(crate) mod nodetest; -mod stream; +pub mod quarantine; +pub(crate) mod stream; pub mod summary; pub mod upload; @@ -63,6 +64,9 @@ pub struct TestReport { /// (`cd ../platform && mix test`). Test identities use it, so a test /// keeps its id whichever shard runs it. pub project: Option, + /// Failing tests the quarantine let through, when it let the target + /// pass. + pub quarantined: Vec, } /// A test that failed in one attempt and passed in a later one. @@ -502,6 +506,7 @@ mod tests { message: None, }, ], + quarantined: Vec::new(), }; assert_eq!(report.failures(), 1); } diff --git a/src/test_results/quarantine.rs b/src/test_results/quarantine.rs new file mode 100644 index 0000000..b9a4e21 --- /dev/null +++ b/src/test_results/quarantine.rs @@ -0,0 +1,911 @@ +//! Quarantine: letting a target pass when its only failures are tests known +//! to be flaky. +//! +//! The list has two sources: +//! +//! - aster-srv's (`GET /v1/test-results/quarantine`), fetched once when a +//! run starts and cached in `.aster/quarantine.json`. When the server +//! cannot be reached, a cached list younger than 24 hours stands in; +//! otherwise there is none. +//! - `aster-quarantine.toml` at the workspace root, reviewed like any other +//! change. Each entry needs a `reason` and an `expires` date and is +//! ignored once that date passes. +//! +//! A failed target reports passed, with the tests it let through, only when +//! all of these hold ([`Quarantine::judge`]): +//! +//! 1. its captured results are `complete`; +//! 2. after retries, every test whose last attempt failed is quarantined, +//! and there is at least one; +//! 3. it exited with its runner's "tests failed" code, not a crash, a +//! compile error, a timeout or a signal; +//! 4. `ASTER_QUARANTINE` is not `off`. +//! +//! Quarantined tests still run and their results are still uploaded. + +use std::collections::HashSet; +use std::path::{Path, PathBuf}; +use std::time::Duration; + +use anyhow::{bail, Context, Result}; +use chrono::{DateTime, NaiveDate, Utc}; +use serde::{Deserialize, Serialize}; + +use super::client::Server; +use super::upload::{proxy_token_from_env, Credentials}; +use super::{same_test, TestCase, TestReport}; +use crate::test_results::Confidence; + +/// The repository's own list, at the workspace root. +pub const REPO_FILE: &str = "aster-quarantine.toml"; + +/// How long a cached server list may stand in for the server. +const CACHE_HOURS: i64 = 24; + +/// How long the start of a run waits for the server's list. +const FETCH_TIMEOUT: Duration = Duration::from_secs(10); + +/// One entry of [`REPO_FILE`]. It names a test by `id` (as the dashboard +/// shows it), or by `name` with any of `file`, `suite` and `target` to +/// narrow it. +#[derive(Debug, Clone, PartialEq, Eq, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct RepoEntry { + #[serde(default)] + pub id: Option, + #[serde(default)] + pub name: Option, + #[serde(default)] + pub file: Option, + #[serde(default)] + pub suite: Option, + /// A project (`//services/platform`) or one of its targets + /// (`//services/platform:test`). + #[serde(default)] + pub target: Option, + pub reason: String, + /// The last day the entry applies, a TOML date such as `2026-11-30`. + #[serde(deserialize_with = "toml_date")] + pub expires: NaiveDate, +} + +fn toml_date<'de, D: serde::Deserializer<'de>>(deserializer: D) -> Result { + let value = toml::value::Datetime::deserialize(deserializer)?; + match (value.date, value.time) { + (Some(date), None) => NaiveDate::from_ymd_opt( + i32::from(date.year), + u32::from(date.month), + u32::from(date.day), + ) + .ok_or_else(|| serde::de::Error::custom("not a calendar date")), + _ => Err(serde::de::Error::custom( + "expires must be a date such as 2026-11-30", + )), + } +} + +#[derive(Deserialize)] +#[serde(deny_unknown_fields)] +struct RepoFile { + #[serde(default)] + test: Vec, +} + +impl RepoEntry { + fn matches(&self, address: &str, test_id: &str, case: &TestCase) -> bool { + if let Some(id) = &self.id { + return id == test_id; + } + let project = address.rsplit_once(':').map_or(address, |(p, _)| p); + self.name.as_deref() == Some(case.name.as_str()) + && self + .file + .as_ref() + .is_none_or(|file| case.file.as_deref() == Some(file.as_str())) + && self + .suite + .as_ref() + .is_none_or(|suite| case.suite.as_deref() == Some(suite.as_str())) + && self + .target + .as_ref() + .is_none_or(|target| target == address || target == project) + } + + fn describe(&self) -> String { + self.id + .clone() + .or_else(|| self.name.clone()) + .unwrap_or_default() + } +} + +/// Read [`REPO_FILE`] from `root`: its entries still in force on `today`, +/// and a warning for each that has expired. No file is no entries. +pub fn read_repo_file(root: &Path, today: NaiveDate) -> Result<(Vec, Vec)> { + let path = root.join(REPO_FILE); + let text = match std::fs::read_to_string(&path) { + Ok(text) => text, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => { + return Ok((Vec::new(), Vec::new())) + } + Err(error) => return Err(error).with_context(|| format!("cannot read {REPO_FILE}")), + }; + let file: RepoFile = toml::from_str(&text).with_context(|| { + format!( + "{REPO_FILE} must hold [[test]] entries with a reason, an expires date, \ + and an id or a name" + ) + })?; + let mut live = Vec::new(); + let mut warnings = Vec::new(); + for (index, entry) in file.test.into_iter().enumerate() { + let number = index + 1; + if entry.reason.trim().is_empty() { + bail!("{REPO_FILE}: test {number} needs a reason"); + } + if entry.id.is_some() == entry.name.is_some() { + bail!("{REPO_FILE}: test {number} needs either an id or a name, not both"); + } + if entry.id.is_some() + && (entry.file.is_some() || entry.suite.is_some() || entry.target.is_some()) + { + bail!("{REPO_FILE}: test {number} names an id, so file, suite and target say nothing"); + } + if entry.expires < today { + warnings.push(format!( + "{REPO_FILE}: the entry for {} expired on {}; it no longer applies", + entry.describe(), + entry.expires + )); + continue; + } + live.push(entry); + } + Ok((live, warnings)) +} + +/// The server's list as cached in `.aster/quarantine.json`. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct ServerList { + /// The server it came from. + pub url: String, + pub repo: String, + pub version: i64, + #[serde(with = "rfc3339")] + pub fetched_at: DateTime, + pub test_ids: Vec, +} + +mod rfc3339 { + use chrono::{DateTime, Utc}; + use serde::{Deserialize, Deserializer, Serializer}; + + pub fn serialize(at: &DateTime, s: S) -> Result { + s.serialize_str(&at.to_rfc3339()) + } + + pub fn deserialize<'de, D: Deserializer<'de>>(d: D) -> Result, D::Error> { + let text = String::deserialize(d)?; + DateTime::parse_from_rfc3339(&text) + .map(|at| at.with_timezone(&Utc)) + .map_err(serde::de::Error::custom) + } +} + +impl ServerList { + /// From the body of `GET /v1/test-results/quarantine`. + fn from_reply(url: &str, reply: &serde_json::Value, now: DateTime) -> Result { + let tests = reply["tests"] + .as_array() + .context("the quarantine list has no tests")?; + Ok(Self { + url: url.to_string(), + repo: reply["repo"].as_str().unwrap_or_default().to_string(), + version: reply["version"] + .as_i64() + .context("the quarantine list has no version")?, + fetched_at: now, + test_ids: tests + .iter() + .filter_map(|test| test["test_id"].as_str().map(str::to_string)) + .collect(), + }) + } +} + +fn cache_path(workspace_root: &Path) -> PathBuf { + workspace_root.join(".aster").join("quarantine.json") +} + +/// Where the server list came from, for the line printed at start. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum ServerSource { + /// Just fetched. + Fetched, + /// The server could not be reached (the reason); the cache stood in. + Cached(String), +} + +/// The tests a run lets fail. +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub struct Quarantine { + pub server: Option<(ServerList, ServerSource)>, + server_ids: HashSet, + pub entries: Vec, +} + +/// What [`Quarantine::judge`] decided for a failed target. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Verdict { + /// Every failure is quarantined: the target passes. The tests, with + /// where their quarantine comes from. + Pass(Vec), + /// Some failures are quarantined, but the target still fails, for the + /// reason given. + Fail(String), + /// None of its failures is quarantined. + Untouched, +} + +/// A failing test the quarantine let through. +#[derive(Debug, Clone, PartialEq, Eq, Serialize)] +pub struct Quarantined { + pub name: String, + #[serde(skip_serializing_if = "Option::is_none")] + pub suite: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub file: Option, + /// `aster-srv`, or the repository file's reason. + pub source: String, +} + +impl Quarantine { + pub fn new(server: Option<(ServerList, ServerSource)>, entries: Vec) -> Self { + let server_ids = server + .as_ref() + .map(|(list, _)| list.test_ids.iter().cloned().collect()) + .unwrap_or_default(); + Self { + server, + server_ids, + entries, + } + } + + pub fn is_empty(&self) -> bool { + self.server_ids.is_empty() && self.entries.is_empty() + } + + /// Why `case` (of the target at `address`, recorded under `project` + /// when its report names one) is quarantined, if it is. + pub fn source_of( + &self, + address: &str, + project: Option<&str>, + case: &TestCase, + ) -> Option { + let test_id = super::stream::test_id(address, project, case); + if self.server_ids.contains(&test_id) { + return Some("aster-srv".to_string()); + } + self.entries + .iter() + .find(|entry| entry.matches(address, &test_id, case)) + .map(|entry| format!("{REPO_FILE}: {}", entry.reason)) + } + + /// Whether a failed target may pass. `plugin` names the target's + /// project plugin, `command` is the target's one command as written + /// (several commands are never judged), `exit_code` its last exit code, + /// and `report` its results after retries. + pub fn judge( + &self, + address: &str, + plugin: &str, + command: &str, + exit_code: Option, + report: &TestReport, + ) -> Verdict { + let failing = last_failures(&report.cases); + let mut quarantined = Vec::new(); + let mut unquarantined = 0; + for case in &failing { + match self.source_of(address, report.project.as_deref(), case) { + Some(source) => quarantined.push(Quarantined { + name: case.name.clone(), + suite: case.suite.clone(), + file: case.file.clone(), + source, + }), + None => unquarantined += 1, + } + } + if quarantined.is_empty() { + return Verdict::Untouched; + } + let of = format!("{} of {} failing tests", quarantined.len(), failing.len()); + if unquarantined > 0 { + return Verdict::Fail(format!("{of} are quarantined; the others still fail")); + } + if report.confidence != Confidence::Complete { + return Verdict::Fail(format!( + "{of} are quarantined, but the results are partial, so other failures may be hidden" + )); + } + let codes = tests_failed_codes(plugin, &report.capture, command); + match exit_code { + Some(code) if codes.contains(&code) => Verdict::Pass(quarantined), + _ if codes.is_empty() => Verdict::Fail(format!( + "{of} are quarantined, but Aster cannot tell this runner's test failures from \ + other errors by its exit code" + )), + code => Verdict::Fail(format!( + "{of} are quarantined, but the command exited {}, not with the runner's \ + \"tests failed\" code {}", + code.map_or("by a signal or not at all".to_string(), |c| c.to_string()), + codes + .iter() + .map(i32::to_string) + .collect::>() + .join(" or ") + )), + } + } + + /// The line printed when a run starts. + pub fn describe(&self) -> String { + let mut parts = Vec::new(); + match &self.server { + Some((list, ServerSource::Fetched)) => parts.push(format!( + "{} from aster-srv (version {})", + list.test_ids.len(), + list.version + )), + Some((list, ServerSource::Cached(why))) => parts.push(format!( + "{} from aster-srv (version {}, cached {}; {why})", + list.test_ids.len(), + list.version, + list.fetched_at.format("%Y-%m-%d %H:%M UTC"), + )), + None => {} + } + if !self.entries.is_empty() { + parts.push(format!("{} from {REPO_FILE}", self.entries.len())); + } + if parts.is_empty() { + "no tests".to_string() + } else { + parts.join(", ") + } + } +} + +/// The tests whose last attempt failed, once each. +fn last_failures(cases: &[TestCase]) -> Vec<&TestCase> { + let mut failing: Vec<&TestCase> = Vec::new(); + for case in cases { + let last = cases + .iter() + .filter(|other| same_test(other, case)) + .map(|other| other.attempt) + .max(); + if case.outcome.is_failure() + && last == Some(case.attempt) + && !failing.iter().any(|known| same_test(known, case)) + { + failing.push(case); + } + } + failing +} + +/// The exit codes with which a runner says "tests failed, nothing else +/// went wrong". Empty when a runner uses one code for both. +pub fn tests_failed_codes(plugin: &str, capture: &str, command: &str) -> &'static [i32] { + // Gradle and Maven exit 1 for compile errors too, however the command + // is spelled. + if matches!(plugin, "gradle" | "maven") { + return &[]; + } + let programs: Vec<&str> = command + .split(|c: char| c.is_whitespace() || c == '\'' || c == '"' || c == ';' || c == '&') + .map(|word| word.rsplit('/').next().unwrap_or(word)) + .collect(); + let runs = |name: &str| programs.contains(&name); + match capture { + // ExUnit exits 2 for failed tests and 1 for a compile error. + "exunit" => &[2], + // A build failure makes go-json results partial, so 1 is tests. + "go-json" | "jest-json" | "node-test" => &[1], + "junit" if runs("nextest") => &[100], + "junit" if ["gradle", "gradlew", "mvn", "mvnw"].iter().any(|p| runs(p)) => &[], + // pytest: 1 is failed tests, 2+ is interrupted or broken. vitest + // and bun: 1. + "junit" => &[1], + // cargo test exits 101 for a failed test and for a compile error. + _ => &[], + } +} + +/// Load the quarantine for a run: `None` when `ASTER_QUARANTINE=off`, or +/// when there is no list at all. Prints what it found, and why the server's +/// list is missing when it is. +pub fn load( + workspace_root: &Path, + env: &dyn Fn(&str) -> Option, +) -> Result> { + if env("ASTER_QUARANTINE").is_some_and(|value| value.eq_ignore_ascii_case("off")) { + eprintln!("[aster] Quarantine: off (ASTER_QUARANTINE=off)"); + return Ok(None); + } + let now = Utc::now(); + let (entries, warnings) = read_repo_file(workspace_root, now.date_naive())?; + for warning in warnings { + eprintln!("[aster] Warning: {warning}"); + } + let server = env("ASTER_SRV_URL") + .filter(|url| !url.is_empty()) + .and_then(|url| { + let fetched = fetch(&url, env, now); + server_list(workspace_root, &url, fetched, now) + }); + let quarantine = Quarantine::new(server, entries); + if quarantine.is_empty() && quarantine.server.is_none() { + return Ok(None); + } + eprintln!("[aster] Quarantine: {}", quarantine.describe()); + Ok(Some(quarantine)) +} + +fn fetch( + url: &str, + env: &dyn Fn(&str) -> Option, + now: DateTime, +) -> Result { + let credentials = Credentials::from_env(env).context( + "no credentials: set ASTER_SRV_TOKEN, or give the job `permissions: id-token: write`", + )?; + let server = + Server::connect_with_timeout(url, &credentials, proxy_token_from_env(env), FETCH_TIMEOUT)?; + let reply = server + .get_json("/v1/test-results/quarantine")? + .context("the server has no quarantine list (is it older than this Aster?)")?; + ServerList::from_reply(url, &reply, now) +} + +/// The server's list: `fetched` when it worked (and cached), else a cache +/// younger than [`CACHE_HOURS`] from the same server. +fn server_list( + workspace_root: &Path, + url: &str, + fetched: Result, + now: DateTime, +) -> Option<(ServerList, ServerSource)> { + let path = cache_path(workspace_root); + match fetched { + Ok(list) => { + let written = std::fs::create_dir_all(path.parent().expect("a parent")) + .map_err(anyhow::Error::from) + .and_then(|()| Ok(serde_json::to_vec(&list)?)) + .and_then(|json| Ok(std::fs::write(&path, json)?)); + if let Err(error) = written { + eprintln!("[aster] Warning: cannot cache the quarantine list: {error:#}"); + } + Some((list, ServerSource::Fetched)) + } + Err(error) => { + let why = format!("{error:#}"); + let cached = std::fs::read(&path) + .ok() + .and_then(|bytes| serde_json::from_slice::(&bytes).ok()) + .filter(|list| list.url == url) + .filter(|list| now - list.fetched_at < chrono::Duration::hours(CACHE_HOURS)); + match cached { + Some(list) => Some((list, ServerSource::Cached(why))), + None => { + eprintln!( + "[aster] Quarantine: aster-srv's list is unavailable ({why}) and no \ + list cached in the last {CACHE_HOURS} hours; only {REPO_FILE} applies" + ); + None + } + } + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::test_results::TestOutcome; + + fn case(name: &str, outcome: TestOutcome, attempt: u32) -> TestCase { + TestCase { + file: Some("test/a_test.exs".into()), + suite: Some("A".into()), + name: name.into(), + outcome, + attempt, + duration_ms: None, + message: None, + } + } + + fn report(cases: Vec) -> TestReport { + TestReport { + project: None, + capture: "exunit".into(), + confidence: Confidence::Complete, + cases, + quarantined: Vec::new(), + } + } + + fn today() -> NaiveDate { + NaiveDate::from_ymd_opt(2026, 10, 15).unwrap() + } + + fn entry(name: &str) -> RepoEntry { + RepoEntry { + id: None, + name: Some(name.into()), + file: None, + suite: None, + target: None, + reason: "flaky on CI".into(), + expires: today(), + } + } + + const ADDRESS: &str = "//services/platform:test"; + const MIX: &str = "mix test"; + + #[test] + fn passes_only_when_every_last_failure_is_quarantined() { + let q = Quarantine::new(None, vec![entry("flaky")]); + let only_flaky = report(vec![ + case("flaky", TestOutcome::Fail, 1), + case("fine", TestOutcome::Pass, 1), + ]); + let Verdict::Pass(tests) = q.judge(ADDRESS, "elixir", MIX, Some(2), &only_flaky) else { + panic!("expected a pass"); + }; + assert_eq!(tests[0].name, "flaky"); + assert_eq!(tests[0].source, "aster-quarantine.toml: flaky on CI"); + + let also_real = report(vec![ + case("flaky", TestOutcome::Fail, 1), + case("real", TestOutcome::Error, 1), + ]); + assert!(matches!( + q.judge(ADDRESS, "elixir", MIX, Some(2), &also_real), + Verdict::Fail(why) if why.contains("1 of 2") + )); + // A retry that fixed the other failure leaves only the quarantined one. + let retried = report(vec![ + case("flaky", TestOutcome::Fail, 1), + case("real", TestOutcome::Fail, 1), + case("flaky", TestOutcome::Fail, 2), + case("real", TestOutcome::Pass, 2), + ]); + assert!(matches!( + q.judge(ADDRESS, "elixir", MIX, Some(2), &retried), + Verdict::Pass(tests) if tests.len() == 1 + )); + let unrelated = report(vec![case("real", TestOutcome::Fail, 1)]); + assert_eq!( + q.judge(ADDRESS, "elixir", MIX, Some(2), &unrelated), + Verdict::Untouched + ); + } + + #[test] + fn never_passes_partial_results_or_other_exit_codes() { + let q = Quarantine::new(None, vec![entry("flaky")]); + let mut partial = report(vec![case("flaky", TestOutcome::Fail, 1)]); + partial.confidence = Confidence::Partial; + assert!(matches!( + q.judge(ADDRESS, "elixir", MIX, Some(2), &partial), + Verdict::Fail(why) if why.contains("partial") + )); + let failed = report(vec![case("flaky", TestOutcome::Fail, 1)]); + // A compile error (1), a signal (None) or a timeout are not test failures. + for code in [Some(1), Some(137), None] { + assert!( + matches!( + q.judge(ADDRESS, "elixir", MIX, code, &failed), + Verdict::Fail(_) + ), + "{code:?}" + ); + } + let mut cargo = failed.clone(); + cargo.capture = "libtest".into(); + assert!(matches!( + q.judge(ADDRESS, "rust", "cargo test", Some(101), &cargo), + Verdict::Fail(why) if why.contains("cannot tell") + )); + } + + #[test] + fn knows_each_runners_tests_failed_code() { + assert_eq!(tests_failed_codes("elixir", "exunit", "mix test"), &[2]); + assert_eq!( + tests_failed_codes( + "elixir", + "exunit", + "bash -c 'cd ../platform && MIX_TEST_PARTITION=1 exec mix test'" + ), + &[2] + ); + let none: &[i32] = &[]; + assert_eq!( + tests_failed_codes("rust", "junit", "cargo nextest run"), + &[100] + ); + assert_eq!(tests_failed_codes("python", "junit", "pytest -q"), &[1]); + assert_eq!(tests_failed_codes("java", "junit", "./gradlew test"), none); + assert_eq!( + tests_failed_codes("gradle", "junit", "sh -c 'make test'"), + none + ); + assert_eq!(tests_failed_codes("maven", "junit", "mvn -q test"), none); + assert_eq!(tests_failed_codes("go", "go-json", "go test ./..."), &[1]); + assert_eq!(tests_failed_codes("rust", "libtest", "cargo test"), none); + } + + #[test] + fn server_entries_match_by_test_id() { + let failing = case("flaky", TestOutcome::Fail, 1); + let id = crate::test_results::stream::test_id(ADDRESS, None, &failing); + let list = ServerList { + url: "https://srv".into(), + repo: "o/r".into(), + version: 7, + fetched_at: Utc::now(), + test_ids: vec![id], + }; + let q = Quarantine::new(Some((list, ServerSource::Fetched)), Vec::new()); + assert_eq!( + q.source_of(ADDRESS, None, &failing).as_deref(), + Some("aster-srv") + ); + // The same test under another project is another test. + assert_eq!(q.source_of("//services/other:test", None, &failing), None); + // A shard running another project's suite records it under that + // project, so the list matches it there. + let shard = crate::test_results::stream::test_id( + "//services/shard:test", + Some("//services/platform"), + &failing, + ); + assert_eq!( + shard, + crate::test_results::stream::test_id("//services/platform:test", None, &failing) + ); + assert_eq!( + q.source_of( + "//services/shard:test", + Some("//services/platform"), + &failing + ) + .as_deref(), + Some("aster-srv") + ); + assert_eq!(q.describe(), "1 from aster-srv (version 7)"); + } + + #[test] + fn repo_entries_narrow_by_file_suite_and_target() { + let failing = case("flaky", TestOutcome::Fail, 1); + let id = crate::test_results::stream::test_id(ADDRESS, None, &failing); + let by = |change: &dyn Fn(&mut RepoEntry)| { + let mut e = entry("flaky"); + change(&mut e); + e.matches(ADDRESS, &id, &failing) + }; + assert!(by(&|_| {})); + assert!(by(&|e| e.file = Some("test/a_test.exs".into()))); + assert!(!by(&|e| e.file = Some("test/b_test.exs".into()))); + assert!(by(&|e| e.suite = Some("A".into()))); + assert!(!by(&|e| e.suite = Some("B".into()))); + assert!(by(&|e| e.target = Some("//services/platform".into()))); + assert!(by(&|e| e.target = Some(ADDRESS.into()))); + assert!(!by(&|e| e.target = Some("//services/plat".into()))); + assert!(by(&|e| { + e.name = None; + e.id = Some(id.clone()); + })); + } + + #[test] + fn reads_the_repo_file_and_drops_expired_entries() { + let dir = tempfile::tempdir().unwrap(); + assert_eq!(read_repo_file(dir.path(), today()).unwrap().0, Vec::new()); + std::fs::write( + dir.path().join(REPO_FILE), + r#" +[[test]] +name = "creates an agent" +file = "test/platform/agents_test.exs" +reason = "DBConnection.OwnershipError, #123" +expires = 2026-10-15 + +[[test]] +id = "abc" +reason = "old" +expires = 2026-10-14 +"#, + ) + .unwrap(); + let (live, warnings) = read_repo_file(dir.path(), today()).unwrap(); + assert_eq!(live.len(), 1); + assert_eq!(live[0].name.as_deref(), Some("creates an agent")); + assert_eq!(warnings.len(), 1); + assert!(warnings[0].contains("abc") && warnings[0].contains("2026-10-14")); + + for bad in [ + "[[test]]\nname = \"x\"\nexpires = 2026-11-01\n", + "[[test]]\nname = \"x\"\nreason = \"r\"\n", + "[[test]]\nname = \"x\"\nreason = \" \"\nexpires = 2026-11-01\n", + "[[test]]\nreason = \"r\"\nexpires = 2026-11-01\n", + "[[test]]\nid = \"a\"\nname = \"x\"\nreason = \"r\"\nexpires = 2026-11-01\n", + "[[test]]\nid = \"a\"\nfile = \"f\"\nreason = \"r\"\nexpires = 2026-11-01\n", + "[[test]]\nname = \"x\"\nreason = \"r\"\nexpires = 2026-11-01\nowner = \"me\"\n", + ] { + std::fs::write(dir.path().join(REPO_FILE), bad).unwrap(); + assert!(read_repo_file(dir.path(), today()).is_err(), "{bad}"); + } + } + + #[test] + fn a_fresh_cache_stands_in_for_an_unreachable_server() { + let dir = tempfile::tempdir().unwrap(); + let now = Utc::now(); + let list = ServerList { + url: "https://srv".into(), + repo: "o/r".into(), + version: 3, + fetched_at: now, + test_ids: vec!["t".into()], + }; + let fetched = server_list(dir.path(), "https://srv", Ok(list.clone()), now); + assert_eq!(fetched, Some((list.clone(), ServerSource::Fetched))); + + let down = || Err(anyhow::anyhow!("connection refused")); + let later = now + chrono::Duration::hours(23); + let Some((cached, ServerSource::Cached(why))) = + server_list(dir.path(), "https://srv", down(), later) + else { + panic!("expected the cache"); + }; + assert_eq!(cached, list); + assert!(why.contains("refused")); + // Too old, or from another server: nothing. + let stale = now + chrono::Duration::hours(25); + assert_eq!(server_list(dir.path(), "https://srv", down(), stale), None); + assert_eq!( + server_list(dir.path(), "https://other", down(), later), + None + ); + } + + #[test] + fn reads_the_servers_reply() { + let reply = serde_json::json!({ + "repo": "o/r", "version": 12, "generated_at": "x", "truncated": false, + "tests": [{"test_id": "a"}, {"test_id": "b"}], + }); + let list = ServerList::from_reply("https://srv", &reply, Utc::now()).unwrap(); + assert_eq!(list.version, 12); + assert_eq!(list.test_ids, vec!["a", "b"]); + assert!(ServerList::from_reply("u", &serde_json::json!({}), Utc::now()).is_err()); + } + + #[test] + fn loads_the_servers_list_then_falls_back_to_its_cache() { + use crate::test_results::client::testing::{closed_port, serve}; + let dir = tempfile::tempdir().unwrap(); + let (url, seen) = serve(vec![( + "/v1/test-results/quarantine", + 200, + r#"{"repo":"o/r","version":9,"tests":[{"test_id":"a"}],"truncated":false}"#.into(), + )]); + let env = |url: String| { + move |name: &str| match name { + "ASTER_SRV_URL" => Some(url.clone()), + "ASTER_SRV_TOKEN" => Some("t0ken".into()), + _ => None, + } + }; + let loaded = load(dir.path(), &env(url.clone())).unwrap().unwrap(); + assert_eq!(loaded.describe(), "1 from aster-srv (version 9)"); + assert!(seen.lock().unwrap()[0].contains("authorization: bearer t0ken")); + assert!(cache_path(dir.path()).exists()); + + // The same server, down: the cache stands in. Another server: no list. + std::fs::write( + cache_path(dir.path()), + std::fs::read_to_string(cache_path(dir.path())) + .unwrap() + .replace(&url, "http://down"), + ) + .unwrap(); + let down = closed_port(); + assert_eq!(load(dir.path(), &env(down)).unwrap(), None); + let cached = server_list( + dir.path(), + "http://down", + Err(anyhow::anyhow!("refused")), + Utc::now(), + ); + assert!(matches!(cached, Some((_, ServerSource::Cached(_))))); + } + + /// The ```toml blocks of a Markdown file that hold `[[test]]` entries. + fn toml_examples(markdown: &str) -> Vec { + markdown + .split("```toml\n") + .skip(1) + .filter_map(|block| block.split("```").next()) + .filter(|block| block.contains("[[test]]")) + .map(str::to_string) + .collect() + } + + #[test] + fn the_documented_quarantine_files_work() { + let docs = [ + include_str!("../../docs/test-tracking.md"), + include_str!("../../README.md"), + ]; + let examples: Vec = docs.iter().flat_map(|d| toml_examples(d)).collect(); + assert_eq!(examples.len(), 2, "both documents show an example"); + let agents = TestCase { + file: Some("test/platform/agents_test.exs".into()), + suite: Some("Platform.AgentsTest".into()), + name: "test creates an agent".into(), + outcome: TestOutcome::Fail, + attempt: 1, + duration_ms: None, + message: None, + }; + for example in examples { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join(REPO_FILE), &example).unwrap(); + let (entries, warnings) = read_repo_file(dir.path(), today()).unwrap(); + assert!(warnings.is_empty(), "{example}"); + let q = Quarantine::new(None, entries); + assert!( + q.source_of("//services/platform:test-ci", None, &agents) + .is_some(), + "{example}" + ); + if example.contains("target =") { + assert!(q + .source_of("//services/other:test-ci", None, &agents) + .is_none()); + } + } + } + + #[test] + fn off_switch_and_no_lists_load_nothing() { + let dir = tempfile::tempdir().unwrap(); + let off = |name: &str| (name == "ASTER_QUARANTINE").then(|| "off".to_string()); + std::fs::write( + dir.path().join(REPO_FILE), + "[[test]]\nname = \"x\"\nreason = \"r\"\nexpires = 2999-01-01\n", + ) + .unwrap(); + assert_eq!(load(dir.path(), &off).unwrap(), None); + let nothing = |_: &str| None; + assert_eq!( + load(dir.path(), ¬hing).unwrap().unwrap().entries.len(), + 1 + ); + std::fs::remove_file(dir.path().join(REPO_FILE)).unwrap(); + assert_eq!(load(dir.path(), ¬hing).unwrap(), None); + } +} diff --git a/src/test_results/stream.rs b/src/test_results/stream.rs index 0baaed0..5cf6f25 100644 --- a/src/test_results/stream.rs +++ b/src/test_results/stream.rs @@ -35,26 +35,24 @@ pub fn records(run: RunRecord, results: &[ExecutionResult]) -> Vec { let Some(report) = &result.tests else { continue; }; - let project = report.project.as_deref().unwrap_or_else(|| { - result - .address - .rsplit_once(':') - .map_or(result.address.as_str(), |(project, _)| project) - }); for case in &report.cases { - let identity = TestIdentity::new( - project, - case.file.clone().unwrap_or_default(), - case.suite.clone().unwrap_or_default(), - case.name.clone(), - ); + // The last attempt of a test the quarantine let through. + let quarantined = case.outcome.is_failure() + && report + .quarantined + .iter() + .any(|q| q.name == case.name && q.suite == case.suite && q.file == case.file) + && !report + .cases + .iter() + .any(|other| super::same_test(other, case) && other.attempt > case.attempt); let message = case .message .as_deref() .filter(|_| case.outcome.is_failure()); records.push(Record::Test(TestRecord { target: result.address.clone(), - test_id: identity.test_id(), + test_id: test_id(&result.address, report.project.as_deref(), case), file: case.file.clone(), suite: case.suite.clone(), name: case.name.clone(), @@ -63,12 +61,31 @@ pub fn records(run: RunRecord, results: &[ExecutionResult]) -> Vec { duration_ms: case.duration_ms, failure_hash: message.map(failure_hash), message: message.map(scrub_message), + quarantined, })); } } records } +/// The test id of `case`, run by the target at `address`: its project, +/// file, suite and name. The project is `project` when the report names one +/// ([`super::TestReport::project`]), else the target's own. +pub(crate) fn test_id(address: &str, project: Option<&str>, case: &super::TestCase) -> String { + let project = project.unwrap_or_else(|| { + address + .rsplit_once(':') + .map_or(address, |(project, _)| project) + }); + TestIdentity::new( + project, + case.file.clone().unwrap_or_default(), + case.suite.clone().unwrap_or_default(), + case.name.clone(), + ) + .test_id() +} + /// Where a results file goes when no path is given: under `$RUNNER_TEMP` /// when set, else the system temp directory, never in the checkout. pub fn default_path(run_id: &str) -> PathBuf { @@ -147,6 +164,44 @@ mod tests { } } + #[test] + fn marks_the_last_attempt_of_a_quarantined_failure() { + let mut retried = case("flaky", TestOutcome::Fail, Some("boom")); + retried.attempt = 2; + let report = TestReport { + project: None, + capture: "exunit".into(), + confidence: Confidence::Complete, + cases: vec![ + case("flaky", TestOutcome::Fail, Some("boom")), + retried, + case("other", TestOutcome::Fail, Some("boom")), + ], + quarantined: vec![crate::test_results::quarantine::Quarantined { + name: "flaky".into(), + suite: Some("AppTest".into()), + file: Some("test/a_test.exs".into()), + source: "aster-srv".into(), + }], + }; + let records = records(run(), &[result("//app:test", true, Some(report))]); + let marks: Vec<(String, u32, bool)> = records + .iter() + .filter_map(|r| match r { + Record::Test(t) => Some((t.name.clone(), t.attempt, t.quarantined)), + _ => None, + }) + .collect(); + assert_eq!( + marks, + [ + ("flaky".to_string(), 1, false), + ("flaky".to_string(), 2, true), + ("other".to_string(), 1, false) + ] + ); + } + #[test] fn writes_run_then_targets_then_their_tests() { let report = TestReport { @@ -161,6 +216,7 @@ mod tests { Some("boom at line 12 with Bearer abcdefghijklmnop"), ), ], + quarantined: Vec::new(), }; let records = records( run(), @@ -211,6 +267,7 @@ mod tests { capture: "exunit-formatter".into(), confidence: Confidence::Complete, cases: vec![case("fails", TestOutcome::Fail, Some("boom"))], + quarantined: Vec::new(), }; records(run(), &[result(shard, false, Some(report))]) }; diff --git a/src/test_results/summary.rs b/src/test_results/summary.rs index 72c4ddf..e2e476b 100644 --- a/src/test_results/summary.rs +++ b/src/test_results/summary.rs @@ -10,6 +10,9 @@ //! - `new`: the server calls it healthy, or has never seen it //! - `failed`: no label, because the server could not be asked //! +//! A failing test that Aster let through because it is quarantined is +//! listed as `quarantined` and not looked up. +//! //! The report never fails the build: a lookup problem only costs labels. use std::collections::HashMap; @@ -43,6 +46,8 @@ pub enum Label { since_sha: Option, }, KnownFlake, + /// Failed, but quarantined: its target passed anyway. + Quarantined, /// Failed, then passed on retry. FlakyOnRetry, } @@ -55,21 +60,29 @@ impl Label { Label::New => "new", Label::Broken { .. } => "broken", Label::KnownFlake => "known flake", + Label::Quarantined => "quarantined", Label::FlakyOnRetry => "flaky on retry", } } - /// Position in the report: new first, then broken, known flakes, and - /// the tests that passed on retry last. + /// Position in the report: new first, then broken, known flakes, + /// quarantined tests, and the tests that passed on retry last. fn rank(&self) -> u8 { match self { Label::New => 0, Label::Failed => 1, Label::Broken { .. } => 2, Label::KnownFlake => 3, - Label::FlakyOnRetry => 4, + Label::Quarantined => 4, + Label::FlakyOnRetry => 5, } } + + /// Whether the test failed its target. Quarantined and flaky-on-retry + /// tests did not. + fn fails(&self) -> bool { + !matches!(self, Label::Quarantined | Label::FlakyOnRetry) + } } /// One test in the report. @@ -105,9 +118,7 @@ impl Report { } fn failing(&self) -> impl Iterator { - self.entries - .iter() - .filter(|entry| entry.label != Label::FlakyOnRetry) + self.entries.iter().filter(|entry| entry.label.fails()) } } @@ -149,7 +160,9 @@ pub fn collect(records: &[Record]) -> Report { .iter() .max_by_key(|test| test.attempt) .expect("a test has an attempt"); - let label = if last.outcome.is_failure() { + let label = if last.outcome.is_failure() && last.quarantined { + Label::Quarantined + } else if last.outcome.is_failure() { Label::Failed } else if last.outcome == aster_results_model::TestOutcome::Pass && tests.iter().any(|test| test.outcome.is_failure()) @@ -308,15 +321,16 @@ fn truncate(text: &str, chars: usize) -> String { } /// The workflow commands for the report: an error per failing test and, when -/// `flaky` is set, a warning per flaky-on-retry test. +/// `flaky` is set, a warning per flaky-on-retry or quarantined test. pub fn annotations(report: &Report, flaky: bool) -> Vec { let mut lines = Vec::new(); for entry in &report.entries { let flaky_on_retry = entry.label == Label::FlakyOnRetry; - if flaky_on_retry && !flaky { + let warned = !entry.label.fails(); + if warned && !flaky { continue; } - let level = if flaky_on_retry { "warning" } else { "error" }; + let level = if warned { "warning" } else { "error" }; let mut properties = Vec::new(); if let Some(path) = annotation_path(entry) { properties.push(format!("file={}", escape_property(&path))); @@ -345,6 +359,7 @@ pub fn annotations(report: &Report, flaky: bool) -> Vec { Label::Broken { since_sha: Some(sha), } => format!("broken since {sha}\n{text}"), + Label::Quarantined => format!("quarantined, so its target passed\n{text}"), _ => text, } }; @@ -386,6 +401,7 @@ fn code(text: &str) -> String { pub fn markdown(report: &Report, url: Option<&str>, problem: Option<&str>) -> String { let failing = report.failing().count(); let flaky = report.count(Label::FlakyOnRetry.rank()); + let quarantined = report.count(Label::Quarantined.rank()); let mut out = String::from("## Aster test results\n\n"); if report.is_empty() { out.push_str("No failing tests.\n"); @@ -405,6 +421,11 @@ pub fn markdown(report: &Report, url: Option<&str>, problem: Option<&str>) -> St if unlabelled > 0 && unlabelled < failing { counts.push(format!("{unlabelled} unlabelled")); } + if quarantined > 0 { + counts.push(format!( + "{quarantined} quarantined (did not fail the build)" + )); + } if flaky > 0 { counts.push(format!("{flaky} flaky on retry")); } @@ -561,9 +582,44 @@ mod tests { duration_ms: None, failure_hash: None, message: message.map(str::to_string), + quarantined: false, }) } + #[test] + fn quarantined_failures_are_listed_but_not_failures() { + let Record::Test(mut held) = test("held", TestOutcome::Fail, 1, Some("boom")) else { + unreachable!() + }; + held.quarantined = true; + let mut report = collect(&[ + run_record(), + Record::Test(held), + test("real", TestOutcome::Fail, 1, Some("bad")), + ]); + assert_eq!(report.entries[0].label, Label::Quarantined); + // Only the real failure is looked up. + let mut asked = Vec::new(); + label(&mut report, &mut |id| { + asked.push(id.to_string()); + Ok(None) + }); + assert_eq!(asked, ["real"]); + sort(&mut report); + let text = markdown(&report, None, None); + assert!(text.contains("**1 failing test**"), "{text}"); + assert!( + text.contains("1 quarantined (did not fail the build)"), + "{text}" + ); + assert!(text.contains("| quarantined | test held"), "{text}"); + let errors = annotations(&report, false); + assert_eq!(errors.len(), 1); + assert!(errors[0].starts_with("::error")); + let all = annotations(&report, true); + assert!(all[1].starts_with("::warning") && all[1].contains("quarantined")); + } + fn sample() -> Vec { use TestOutcome::*; vec![ diff --git a/src/ui/progress.rs b/src/ui/progress.rs index cb757cd..680aae1 100644 --- a/src/ui/progress.rs +++ b/src/ui/progress.rs @@ -45,6 +45,7 @@ pub struct ProgressDisplay { running: Arc, /// Targets that passed only on a retry, by address: how many tests flaky: HashMap, + quarantined: HashMap, } impl ProgressDisplay { @@ -87,6 +88,7 @@ impl ProgressDisplay { cached: Arc::new(AtomicUsize::new(0)), running: Arc::new(AtomicUsize::new(0)), flaky: HashMap::new(), + quarantined: HashMap::new(), } } @@ -243,6 +245,12 @@ impl ProgressDisplay { self.flaky.insert(address.to_string(), tests); } + /// Note that a target passed only because `tests` failing tests are + /// quarantined. Call before [`Self::mark_complete`]. + pub fn mark_quarantined(&mut self, address: &str, tests: usize) { + self.quarantined.insert(address.to_string(), tests); + } + /// Mark a target as complete /// /// Updates counts and removes spinner. Behavior differs by mode: @@ -271,7 +279,8 @@ impl ProgressDisplay { } let flaky = self.flaky.remove(address).filter(|_| success); - let flaky_note = flaky + let quarantined = self.quarantined.remove(address).filter(|_| success); + let mut flaky_note = flaky .map(|tests| { format!( " {} flaky: {tests} test{} passed on retry", @@ -280,6 +289,13 @@ impl ProgressDisplay { ) }) .unwrap_or_default(); + if let Some(tests) = quarantined { + flaky_note.push_str(&format!( + " {} (quarantined: {tests})", + style("\u{2014}").dim() + )); + } + let flaky = flaky.or(quarantined); // CI mode: print PASS/FAIL with timestamp and duration if self.ci_mode { diff --git a/tests/test_results.rs b/tests/test_results.rs index 32365f7..375aa9d 100644 --- a/tests/test_results.rs +++ b/tests/test_results.rs @@ -702,3 +702,170 @@ fn shards_split_by_the_committed_durations_file_without_a_server() { "test test/b_test.exs test/c_test.exs test/d_test.exs\n" ); } + +// Quarantine, end to end with a real `go test`. + +/// A Go module with a test that always fails (`TestQuarantined`), one that +/// passes, and, when `also_failing`, a second failing test. `after` depends +/// on `test` and records that it ran. +fn quarantine_go_workspace(also_failing: bool, quarantine_file: &str) -> TempDir { + let tmp = TempDir::new().unwrap(); + fs::create_dir(tmp.path().join(".git")).unwrap(); + write( + tmp.path(), + "app/go.mod", + "module example.com/app\n\ngo 1.21\n", + ); + let other = if also_failing { + "\nfunc TestReal(t *testing.T) { t.Fatal(\"a real failure\") }\n" + } else { + "" + }; + write( + tmp.path(), + "app/app_test.go", + &format!( + "package app\n\nimport \"testing\"\n\n\ + func TestFine(t *testing.T) {{}}\n\n\ + func TestQuarantined(t *testing.T) {{ t.Fatal(\"flaky on CI\") }}\n{other}" + ), + ); + write( + tmp.path(), + "app/aster.toml", + "[targets.deps]\ncommand = \"true\"\n\n[targets.test]\ncommand = \"go test ./...\"\n\n\ + [targets.after]\ncommand = \"touch ../after.ran\"\ndepends_on = [\"//self:test\"]\n", + ); + if !quarantine_file.is_empty() { + write(tmp.path(), "aster-quarantine.toml", quarantine_file); + } + tmp +} + +const QUARANTINE_FILE: &str = r#" +[[test]] +name = "TestQuarantined" +target = "//app" +reason = "flaky on CI, see #1" +expires = 2999-01-01 +"#; + +#[test] +fn a_target_whose_only_failures_are_quarantined_passes_and_says_so() { + if !go_available() { + eprintln!("skipping: go is not installed"); + return; + } + let tmp = quarantine_go_workspace(false, QUARANTINE_FILE); + let results = tmp.path().join("out/results.ndjson"); + let step = tmp.path().join("step-summary.md"); + let output = aster_in_github( + tmp.path(), + &step, + &[ + "--no-cache", + &format!("--test-results={}", results.display()), + "run", + "//app:after", + ], + ); + let stderr = String::from_utf8_lossy(&output.stderr); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!(output.status.success(), "{stderr}\n{stdout}"); + assert!( + stderr.contains("[aster] Quarantine: 1 from aster-quarantine.toml"), + "{stderr}" + ); + assert!(stderr.contains("(quarantined: 1)"), "{stderr}"); + // Go names a test's package, not its file. + assert!( + stderr.contains("::warning::TestQuarantined failed but is quarantined"), + "{stderr}" + ); + // The dependent target ran. + assert!(tmp.path().join("after.ran").exists()); + + let records = read_records(&results); + assert_eq!(target_status(&records, "//app:test"), TargetStatus::Passed); + let quarantined: Vec<_> = records + .iter() + .filter_map(|r| match r { + Record::Test(t) if t.quarantined => Some(t.name.as_str()), + _ => None, + }) + .collect(); + assert_eq!(quarantined, ["TestQuarantined"]); + let summary = fs::read_to_string(&step).unwrap(); + assert!(summary.contains("1 quarantined"), "{summary}"); + assert!( + summary.contains("| quarantined | TestQuarantined"), + "{summary}" + ); + // The run already warned; the summary adds no error for it. + assert!(!stderr.contains("::error"), "{stderr}"); +} + +#[test] +fn any_other_failure_still_fails_the_target() { + if !go_available() { + eprintln!("skipping: go is not installed"); + return; + } + let tmp = quarantine_go_workspace(true, QUARANTINE_FILE); + let output = aster( + tmp.path(), + &["--no-cache", "--test-results=out/r.ndjson", "test", "//app"], + ); + assert!(!output.status.success()); + let all = format!( + "{}{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + assert!( + all.contains("1 of 2 failing tests are quarantined; the others still fail"), + "{all}" + ); +} + +#[test] +fn quarantine_can_be_turned_off_and_needs_captured_results() { + if !go_available() { + eprintln!("skipping: go is not installed"); + return; + } + let tmp = quarantine_go_workspace(false, QUARANTINE_FILE); + let off = Command::new(env!("CARGO_BIN_EXE_aster")) + .current_dir(tmp.path()) + .env("ASTER_QUARANTINE", "off") + .env_remove("ASTER_SRV_URL") + .env("RUNNER_TEMP", tmp.path().join("runner-temp")) + .args(["--no-cache", "--test-results=out/r.ndjson", "test", "//app"]) + .output() + .unwrap(); + assert!(!off.status.success()); + assert!(String::from_utf8_lossy(&off.stderr).contains("Quarantine: off")); + // Without --test-results or --retry-failed nothing is captured, so + // nothing is let through. + let plain = aster(tmp.path(), &["--no-cache", "test", "//app"]); + assert!(!plain.status.success()); +} + +#[test] +fn a_bad_quarantine_file_is_an_error() { + if !go_available() { + eprintln!("skipping: go is not installed"); + return; + } + let tmp = quarantine_go_workspace(false, "[[test]]\nname = \"TestQuarantined\"\n"); + let output = aster( + tmp.path(), + &["--no-cache", "--test-results=out/r.ndjson", "test", "//app"], + ); + assert!(!output.status.success()); + assert!( + String::from_utf8_lossy(&output.stderr).contains("aster-quarantine.toml"), + "{}", + String::from_utf8_lossy(&output.stderr) + ); +}