From 7c091c754c8701ba570db671fd1465906881c7ed Mon Sep 17 00:00:00 2001 From: Julien Danjou Date: Fri, 4 Sep 2026 16:38:47 +0200 Subject: [PATCH 1/2] docs(live-tests): drop the mirror claims for deleted Python files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The module header promised this file "deliberately mirrors the Python version 1:1 ... so the port can't drift the contract by accident", and named `func-tests/test_live_smoke.py` and `func-tests/conftest.py` as what it mirrors. Both were deleted when the port completed. Four helpers made the same promise individually (`Mirrors conftest.py::cli`, `Mirrors Python subprocess.run`, `Matches Python result.stdout + result.stderr`, and `live_token`'s "Mirrors Python `live_token` fixture"). AGENTS.md: "There is no Python: the port is complete ... If you find a doc, comment, or rule mentioning [it], it is stale — fix it." The harm is concrete rather than cosmetic: a maintainer auditing which secret each test needs goes looking for the fixture the header says pins that mapping, and there isn't one. The header now states the invariant that actually holds — tests are grouped by credential under a banner, and a test belongs under the banner matching its helper — which is the thing a reader needs and the thing the previous commit had to correct. It also spells `LIVE_TEST_MERGIFY_TOKEN_ADMIN` out rather than abbreviating it to `_ADMIN`, so grepping for either secret finds this file. Left alone: the in-body notes recording which wire contracts were preserved across the Python → Rust port. Those are provenance for why a contract is shaped the way it is, not claims about a file that no longer exists. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JBz3hxMUDCuWftT6qAHtnn Change-Id: Idd9e1e28bfc50b8b71d07c5938b4fa89af2745ff --- crates/mergify-cli/tests/live_smoke.rs | 36 ++++++++++++-------------- 1 file changed, 17 insertions(+), 19 deletions(-) diff --git a/crates/mergify-cli/tests/live_smoke.rs b/crates/mergify-cli/tests/live_smoke.rs index 08ca9cd6..d35019d1 100644 --- a/crates/mergify-cli/tests/live_smoke.rs +++ b/crates/mergify-cli/tests/live_smoke.rs @@ -1,16 +1,15 @@ -//! Live smoke tests against the real Mergify API. Port of -//! `func-tests/test_live_smoke.py` + `func-tests/conftest.py`. +//! Live smoke tests against the real Mergify API. //! //! Each test fires when the real API's URL, auth, or wire format //! diverges from what the CLI expects. API-hitting tests skip //! (early-return with a `SKIP:` line) unless their token -//! (`LIVE_TEST_MERGIFY_TOKEN_CI` or `_ADMIN`) is set in the env; -//! locally-evaluated tests run unconditionally. Driven by -//! `.github/workflows/func-tests-live.yaml` on every PR. +//! (`LIVE_TEST_MERGIFY_TOKEN_CI` or `LIVE_TEST_MERGIFY_TOKEN_ADMIN`) +//! is set in the env. Locally-evaluated tests run unconditionally. +//! Driven by `.github/workflows/func-tests-live.yaml` on every PR. //! -//! Implementation deliberately mirrors the Python version 1:1 — -//! same scrubbed env list, same assertion messages, same fixture -//! shape — so the port can't drift the contract by accident. +//! Tests are grouped by the credential they need, under a banner +//! per group — a test's banner is the index of which secret it +//! consumes, so keep a test under the one matching its helper. use std::collections::HashSet; use std::path::{Path, PathBuf}; @@ -23,13 +22,13 @@ const API_URL: &str = "https://api.mergify.com"; const REPOSITORY: &str = "mergify-clients-testing/mergify-cli-repo"; const PULL_REQUEST: &str = "1"; -/// CLI invocation timeout. Mirrors Python `subprocess.run(timeout=30)`. +/// CLI invocation timeout. const CLI_TIMEOUT: Duration = Duration::from_secs(30); /// Env vars the CLI auto-detects from the surrounding CI runner. /// Scrub them so a developer running tests inside GitHub Actions /// or Buildkite doesn't get different behavior than a clean -/// laptop run. Mirrors `conftest.py::_CI_ENV_VARS`. +/// laptop run. const CI_ENV_VARS: &[&str] = &[ "CI", "GITHUB_ACTIONS", @@ -63,7 +62,7 @@ struct CliResult { impl CliResult { /// Combined stream for grep-style assertions where the message /// could land on either stdout or stderr depending on the - /// command. Matches Python `result.stdout + result.stderr`. + /// command. fn combined(&self) -> String { format!("{}{}", self.stdout, self.stderr) } @@ -84,10 +83,9 @@ fn mergify_binary() -> &'static Path { /// Run `mergify ` with a scrubbed env and a fresh tmp cwd. /// -/// Mirrors Python `conftest.py::cli` exactly: closes stdin so an -/// accidental interactive prompt fails fast instead of blocking; -/// caps wall-clock at [`CLI_TIMEOUT`] so a pathological hang -/// doesn't drag the CI matrix down with it. +/// Closes stdin so an accidental interactive prompt fails fast +/// instead of blocking. Caps wall-clock at [`CLI_TIMEOUT`] so a +/// pathological hang doesn't drag the CI matrix down with it. /// /// **Concurrency.** Cargo's stock test harness runs every /// `#[test]` in this binary in a single process across a thread @@ -187,10 +185,10 @@ fn wait_timeout( } } -/// Look up `LIVE_TEST_MERGIFY_TOKEN_CI`. Mirrors Python -/// `live_token` fixture — empty / unset = skip the test (early -/// return with `SKIP:` printed to stderr so the cargo test log -/// shows what was skipped). +/// Look up `LIVE_TEST_MERGIFY_TOKEN_CI`, the key scoped to what a +/// CI job does. Empty / unset = skip the test (early return with +/// `SKIP:` printed to stderr so the cargo test log shows what was +/// skipped). fn live_token() -> Option { let token = std::env::var("LIVE_TEST_MERGIFY_TOKEN_CI") .unwrap_or_default() From 4c11a93a8a632a96fcd288d7fb14cda95f84ec59 Mon Sep 17 00:00:00 2001 From: Julien Danjou Date: Fri, 4 Sep 2026 17:37:33 +0200 Subject: [PATCH 2/2] test(live): cover the quarantine add/remove round-trip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MRGFY-9001 dropped `ci_application_key` from `POST` and `DELETE` on `/ci/{owner}/repositories/{repo}/quarantines` at the same time it dropped it from `search/tests`. Only `search/tests` had a live test, so only `search/tests` turned red; `mergify tests quarantines add` and `remove` broke for every `ci` key with no signal at all. The first two commits of this stack documented that. This closes it. The 20-odd wiremock tests in `tests_quarantine.rs` cannot cover this: a mock server never authenticates, so no scope change is visible to them. It has to be the live suite or nothing. Shape follows `freeze_create_update_delete_roundtrip`, the existing create-and-clean-up test in this file: a `Drop` guard removes the entry so a failed assertion mid-test still leaves the canary repository clean, and it warns rather than panics, because panicking in `Drop` during an unwind aborts the process and buries the assertion message that explains the failure. The guard is registered before the response is parsed — an unparseable body still means the row exists server-side. Two details specific to quarantines: - The quarantined name is `__mergify_cli_smoke_quarantine___` and matches nothing any real suite reports, so even a completely skipped cleanup cannot suppress a genuine failure on the canary repository. A leaked row is inert. - Removal goes by name rather than by the id `add` returned, so one run covers the list read that resolves the name *and* the delete. The guard tolerates exactly two outcomes: exit 0, when the body failed before its own remove, and `MergifyApiError` carrying `not_found`'s `'' is not quarantined`, when the body already removed the row. Both halves are load-bearing — that exit code is every Mergify API error including a 403 on a narrowed scope, and the message on its own would swallow any failure whose text happened to contain the phrase. It reads the code off `mergify_core::ExitCode` rather than a literal, so a renumbering cannot silently widen what cleanup calls success. The 8-char entropy the freeze test generated inline is now `unique_suffix()`, shared by both. Co-Authored-By: Claude Opus 5 (1M context) Change-Id: I20ed0098fe86acd633ad6864eed035f6e5f47dd1 --- crates/mergify-cli/tests/live_smoke.rs | 172 ++++++++++++++++++++++--- 1 file changed, 152 insertions(+), 20 deletions(-) diff --git a/crates/mergify-cli/tests/live_smoke.rs b/crates/mergify-cli/tests/live_smoke.rs index d35019d1..67f993f6 100644 --- a/crates/mergify-cli/tests/live_smoke.rs +++ b/crates/mergify-cli/tests/live_smoke.rs @@ -217,6 +217,24 @@ fn live_admin_token() -> Option { (!token.is_empty()).then_some(token) } +/// 8 random hex-ish chars, so concurrent or repeated runs never +/// fight over the same row on the shared canary repository. +/// `tempfile`'s name generation is already a dependency and draws +/// from `getrandom`, so it saves pulling in a uuid crate for this. +fn unique_suffix() -> String { + let dir = tempfile::tempdir().expect("tempdir for entropy"); + let name = dir + .path() + .file_name() + .and_then(|s| s.to_str()) + .unwrap_or("00000000") + .to_string(); + // tempdir names are like ".tmpXXXXXX" — take a tail slice so + // the literal prefix is not included. + let tail: String = name.chars().rev().take(8).collect(); + tail.chars().rev().collect::() +} + /// Helper for "early-return on missing token". Cargo's stock /// test harness doesn't have a "skip" outcome — early-returning /// counts as pass. Logging `SKIP:` to stderr makes the elided @@ -509,25 +527,8 @@ fn freeze_create_update_delete_roundtrip() { let token = skip_if_unset!(live_admin_token()); // Unique reason so concurrent or repeated runs don't fight - // over the same row. The Python suite uses - // `uuid.uuid4().hex[:8]`; reproduce that entropy with - // `tempfile`'s name-generation (32 hex chars from - // `getrandom`), truncated to 8. - let suffix = { - let dir = tempfile::tempdir().expect("tempdir for entropy"); - let name = dir - .path() - .file_name() - .and_then(|s| s.to_str()) - .unwrap_or("00000000") - .to_string(); - // tempdir names are like ".tmpXXXXXX" or "tmpXXXXXX" with - // ~6+ random chars after the prefix. Take a tail slice so - // we don't include the literal prefix. - let tail: String = name.chars().rev().take(8).collect(); - tail.chars().rev().collect::() - }; - let reason = format!("func-tests-live-smoke-{suffix}"); + // over the same row. + let reason = format!("func-tests-live-smoke-{}", unique_suffix()); let create = cli(&[ "freeze", @@ -608,7 +609,7 @@ fn freeze_create_update_delete_roundtrip() { } // --------------------------------------------------------------- -// CI-Insights test reads (admin token). +// CI-Insights tests family (admin token). // --------------------------------------------------------------- #[test] @@ -654,6 +655,137 @@ fn tests_show_no_match() { ); } +/// RAII cleanup for `tests_quarantine_add_remove_roundtrip` — +/// runs `quarantines remove` from `Drop` so a failed assertion +/// mid-test still leaves the canary repository clean. Same +/// warn-don't-panic posture as [`DeleteFreezeOnDrop`]: panicking +/// in `Drop` during an unwind aborts the process and buries the +/// real assertion message. +struct RemoveQuarantineOnDrop<'a> { + token: &'a str, + test_name: &'a str, +} +impl Drop for RemoveQuarantineOnDrop<'_> { + fn drop(&mut self) { + let remove = cli(&[ + "tests", + "quarantines", + "remove", + "--api-url", + API_URL, + "--token", + self.token, + "--repository", + REPOSITORY, + self.test_name, + ]); + // Two outcomes leave the repository clean: exit 0, when + // the body failed before its own remove, and the happy + // path's exit 6 — `MergifyApiError` — carrying + // `not_found`'s `'' is not quarantined`, because the + // body already removed the row. + // + // Both halves are load-bearing. Exit 6 alone is every + // Mergify API error, a 403 on a narrowed scope included; + // the message alone would swallow any failure whose text + // happens to contain that phrase. Anything else is a + // cleanup that did not happen, and the row is still there. + let already_gone = remove.returncode + == i32::from(mergify_core::ExitCode::MergifyApiError.as_u8()) + && remove.combined().contains("is not quarantined"); + if remove.returncode != 0 && !already_gone { + eprintln!("WARNING: quarantine cleanup failed{}", remove.context()); + } + } +} + +#[test] +fn tests_quarantine_add_remove_roundtrip() { + // `POST` + `DELETE` round-trip on + // `/v1/ci/{owner}/repositories/{repo}/quarantines`. + // + // MRGFY-9001 dropped `ci_application_key` from both of these + // alongside `search/tests`, and nothing here noticed, because + // the quarantine mutations had no live coverage at all — the + // wiremock tests in `tests_quarantine.rs` cannot see a scope + // change, since a mock server never authenticates. This is + // that canary: it fires on an auth, URL or wire-format change + // to either endpoint. + // + // Quarantines a name no real suite reports, so the entry can + // never suppress a genuine failure on the canary repository + // even if cleanup is skipped entirely. + let token = skip_if_unset!(live_admin_token()); + + let test_name = format!("__mergify_cli_smoke_quarantine_{}__", unique_suffix()); + let reason = "func-tests-live-smoke"; + + let add = cli(&[ + "tests", + "quarantines", + "add", + "--api-url", + API_URL, + "--token", + &token, + "--repository", + REPOSITORY, + "--reason", + reason, + "--json", + &test_name, + ]); + assert_eq!(add.returncode, 0, "quarantines add failed{}", add.context()); + + // Registered before the payload is parsed: an unparseable + // body still means the row was created server-side. + let _cleanup = RemoveQuarantineOnDrop { + token: &token, + test_name: &test_name, + }; + + let added: Value = serde_json::from_str(&add.stdout).unwrap_or_else(|e| { + panic!( + "quarantines add --json emitted non-JSON output\nerror: {e}\nstdout:\n{}", + add.stdout + ) + }); + assert_eq!( + added["test_name"], + serde_json::json!(test_name), + "add echoed a different test name\nstdout:\n{}", + add.stdout + ); + assert!( + added["id"].as_str().is_some_and(|id| !id.is_empty()), + "add emitted no quarantine id\nstdout:\n{}", + add.stdout + ); + + // Remove by name, which is the path that resolves the id via + // the list endpoint — so one run covers both the `ci`-allowed + // read and the admin-only delete. + let remove = cli(&[ + "tests", + "quarantines", + "remove", + "--api-url", + API_URL, + "--token", + &token, + "--repository", + REPOSITORY, + "--json", + &test_name, + ]); + assert_eq!( + remove.returncode, + 0, + "quarantines remove failed{}", + remove.context() + ); +} + // --------------------------------------------------------------- // CI commands — locally evaluated, no token needed. // ---------------------------------------------------------------