diff --git a/crates/mergify-ci/src/junit_process/command.rs b/crates/mergify-ci/src/junit_process/command.rs index 3780b675..105ce2c3 100644 --- a/crates/mergify-ci/src/junit_process/command.rs +++ b/crates/mergify-ci/src/junit_process/command.rs @@ -474,12 +474,10 @@ fn blocking_fallback(cases: &[TestCase]) -> QuarantineResult { .filter(|c| c.status.is_failure()) .cloned() .collect(); - let count = failing.len(); QuarantineResult { non_quarantined: failing.clone(), failing, quarantined: Vec::new(), - failing_not_quarantined_count: count, } } @@ -493,8 +491,8 @@ fn quarantine_failure_message( "Treating {nb_failures}/{nb_failures} failures as blocking" )); } - if result.failing_not_quarantined_count > 0 { - let count = result.failing_not_quarantined_count; + if !result.non_quarantined.is_empty() { + let count = result.non_quarantined.len(); let total = result.failing.len(); let quarantined = total - count; return Some(format!("{quarantined}/{total} failures quarantined")); @@ -940,7 +938,6 @@ mod tests { failing: vec![case("a", TestStatus::Failed), case("b", TestStatus::Failed)], non_quarantined: vec![case("a", TestStatus::Failed), case("b", TestStatus::Failed)], quarantined: vec![], - failing_not_quarantined_count: 2, }; let msg = quarantine_failure_message(&result, 2, true); // Pythonic phrasing: "Treating X/X failures as blocking". @@ -953,7 +950,6 @@ mod tests { failing: vec![case("a", TestStatus::Failed), case("b", TestStatus::Failed)], quarantined: vec![case("a", TestStatus::Failed)], non_quarantined: vec![case("b", TestStatus::Failed)], - failing_not_quarantined_count: 1, }; // 1/2 still blocking → message says "1/2 quarantined". let msg = quarantine_failure_message(&result, 2, false); @@ -966,7 +962,6 @@ mod tests { failing: vec![case("a", TestStatus::Failed)], quarantined: vec![case("a", TestStatus::Failed)], non_quarantined: vec![], - failing_not_quarantined_count: 0, }; // Every failure quarantined → no failure message. assert_eq!(quarantine_failure_message(&result, 1, false), None); @@ -1202,11 +1197,10 @@ mod tests { // the wire shape is already covered by `quarantine.rs` and // `upload.rs` unit tests. async fn mount_mocks(server: &MockServer) { - Mock::given(method("POST")) - .and(path("/v1/ci/owner/repositories/repo/quarantines/check")) + Mock::given(method("GET")) + .and(path("/v1/ci/owner/repositories/repo/quarantines")) .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ - "quarantined_tests_names": [], - "non_quarantined_tests_names": [], + "quarantined_tests": [], }))) .mount(server) .await; @@ -1494,11 +1488,10 @@ mod tests { #[tokio::test] async fn partial_fan_out_reports_the_session_as_incomplete() { let server = MockServer::start().await; - Mock::given(method("POST")) - .and(path("/v1/ci/owner/repositories/repo/quarantines/check")) + Mock::given(method("GET")) + .and(path("/v1/ci/owner/repositories/repo/quarantines")) .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ - "quarantined_tests_names": [], - "non_quarantined_tests_names": [], + "quarantined_tests": [], }))) .mount(&server) .await; @@ -1976,11 +1969,10 @@ mod tests { // Mount a quarantine mock that says "nothing quarantined" // and a traces mock that answers `upload_status`. async fn mount_mocks_with_upload_status(server: &MockServer, upload_status: u16) { - Mock::given(method("POST")) - .and(path("/v1/ci/owner/repositories/repo/quarantines/check")) + Mock::given(method("GET")) + .and(path("/v1/ci/owner/repositories/repo/quarantines")) .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ - "quarantined_tests_names": [], - "non_quarantined_tests_names": [], + "quarantined_tests": [], }))) .mount(server) .await; diff --git a/crates/mergify-ci/src/junit_process/mod.rs b/crates/mergify-ci/src/junit_process/mod.rs index fce650b6..587abb07 100644 --- a/crates/mergify-ci/src/junit_process/mod.rs +++ b/crates/mergify-ci/src/junit_process/mod.rs @@ -25,7 +25,7 @@ pub mod upload; pub use command::{JunitProcessOptions, run}; pub use junit::{Failure, InvalidJunitXml, ParseResult, TestCase, TestStatus}; -pub use quarantine::{QuarantineFailed, QuarantineResult, QuarantinedTests}; +pub use quarantine::{QuarantineFailed, QuarantineResult}; pub use spans::{BuiltTraces, UploadMetadata, build_traces}; pub use split::{Chunk, SplitOutcome, split_request}; pub use upload::{ diff --git a/crates/mergify-ci/src/junit_process/quarantine.rs b/crates/mergify-ci/src/junit_process/quarantine.rs index 6c5f3b39..91908603 100644 --- a/crates/mergify-ci/src/junit_process/quarantine.rs +++ b/crates/mergify-ci/src/junit_process/quarantine.rs @@ -6,32 +6,27 @@ //! tests still block. //! //! Endpoint shape: -//! `POST {api_url}/v1/ci/{owner}/repositories/{repo}/quarantines/check` +//! `GET {api_url}/v1/ci/{owner}/repositories/{repo}/quarantines?branch=...` +//! returns, one cursor-paginated page at a time, //! ```json -//! { "tests_names": [...], "branch": "..." } -//! ``` -//! returns -//! ```json -//! { "quarantined_tests_names": [...], "non_quarantined_tests_names": [...] } +//! { "quarantined_tests": [{ "test_name": "..." }, ...] } //! ``` use std::collections::BTreeSet; use mergify_core::{ApiFlavor, CliError, HttpClient}; -use serde::{Deserialize, Serialize}; +use serde::Deserialize; use url::Url; use crate::detector; use crate::junit_process::junit::TestCase; +use crate::tests_quarantine::QuarantineList; -/// What the quarantine API told us about a set of failing test -/// case names — sets so membership checks are O(log n) when we -/// later tag each span. -#[derive(Debug, Clone, Default, PartialEq, Eq)] -pub struct QuarantinedTests { - pub quarantined: BTreeSet, - pub non_quarantined: BTreeSet, -} +/// Page size requested from the quarantine list endpoint. +const PER_PAGE: &str = "100"; + +/// Status the API answers when quarantine is not in the plan. +const PAYMENT_REQUIRED: u16 = 402; /// Cross-cutting view of a `junit-process` run: which case names /// failed, which the backend says are currently quarantined, and @@ -43,19 +38,11 @@ pub struct QuarantinedTests { pub struct QuarantineResult { /// Every failing test case (status = Failed or Errored). pub failing: Vec, - /// Subset of `failing` whose names appear in the - /// `quarantined_tests_names` API response. + /// Subset of `failing` currently quarantined on the branch. pub quarantined: Vec, - /// Subset of `failing` the API explicitly reported as - /// non-quarantined. May be a strict subset of - /// `failing - quarantined` when the API silently dropped some - /// names; we trust the API's split rather than reconstructing - /// it locally, to match Python. + /// Subset of `failing` not quarantined on the branch. Empty → + /// CI passes, non-empty → CI fails. pub non_quarantined: Vec, - /// Count of failing tests that are NOT quarantined. This is - /// what determines the final exit code: zero → CI passes, - /// non-zero → CI fails. - pub failing_not_quarantined_count: usize, } #[derive(Debug, Clone)] @@ -74,7 +61,7 @@ impl std::error::Error for QuarantineFailed {} /// Find every test case in `cases` whose status is a failure /// (`Failed` or `Errored`). Mirrors Python's filter; the spans /// inheriting this property are tagged `cicd.test.quarantined` -/// at the `spans` layer based on the result of [`check`]. +/// at the `spans` layer based on the result of [`fetch`]. fn failing_cases(cases: &[TestCase]) -> Vec { cases .iter() @@ -83,70 +70,71 @@ fn failing_cases(cases: &[TestCase]) -> Vec { .collect() } -/// Query the Mergify CI Insights quarantine API for the names in -/// `failing_names` against the given branch. Returns the API's -/// own split — we do NOT reconstruct `non_quarantined = -/// failing - quarantined` locally, to match Python's behavior. -/// -/// `failing_names` may contain duplicates if the `JUnit` input has -/// the same test name reported by multiple suites; that's fine — -/// the API treats names as a set. -pub async fn check( +/// Fetch the names of every test quarantined on `branch`, following +/// the cursor pagination until the last page. A repository without +/// the feature in its plan (402) has nothing quarantined. +pub async fn fetch( api_url: &Url, token: &str, repository: &str, branch: &str, - failing_names: &[String], -) -> Result { - let (owner, repo) = detector::split_owner_repo(repository).map_err(|e| QuarantineFailed { - message: e.to_string(), - })?; - - let client = HttpClient::new(api_url.clone(), token, ApiFlavor::Mergify).map_err(|e| { - QuarantineFailed { - message: e.to_string(), +) -> Result, QuarantineFailed> { + let failed = |message: String| QuarantineFailed { message }; + let (owner, repo) = + detector::split_owner_repo(repository).map_err(|e| failed(e.to_string()))?; + let client = HttpClient::new(api_url.clone(), token, ApiFlavor::Mergify) + .map_err(|e| failed(e.to_string()))?; + + let path = format!("/v1/ci/{owner}/repositories/{repo}/quarantines"); + let mut quarantined = BTreeSet::new(); + let mut seen_cursors = BTreeSet::new(); + let mut cursor: Option = None; + loop { + let mut query = vec![("branch", branch), ("per_page", PER_PAGE)]; + if let Some(cursor) = &cursor { + query.push(("cursor", cursor)); + } + let Some(page) = client + .get_page_unless::>(&path, &query, PAYMENT_REQUIRED) + .await + .map_err(|e| failed(e.to_string()))? + else { + return Ok(BTreeSet::new()); + }; + quarantined.extend(page.body.quarantined_tests.into_iter().map(|t| t.test_name)); + + let Some(next) = page.next_cursor else { + return Ok(quarantined); + }; + // A cursor pointing back to a fetched page would loop forever; + // a partial list would silently un-quarantine tests. + if !seen_cursors.insert(next.clone()) { + return Err(failed( + "quarantine pagination cycled back to a fetched page".to_string(), + )); } - })?; - - let path = format!("/v1/ci/{owner}/repositories/{repo}/quarantines/check"); - let body = CheckRequest { - tests_names: failing_names, - branch, - }; - - let resp: CheckResponse = client - .post(&path, &body) - .await - .map_err(|e| QuarantineFailed { - message: e.to_string(), - })?; - - Ok(QuarantinedTests { - quarantined: resp.quarantined_tests_names.into_iter().collect(), - non_quarantined: resp.non_quarantined_tests_names.into_iter().collect(), - }) + cursor = Some(next); + } } /// Categorize the failing test cases into quarantined and -/// non-quarantined buckets, given the API's verdict. The result +/// non-quarantined buckets, given the branch's quarantined names. The result /// keeps the failing-cases list intact so the CLI can render the /// "X/Y failures quarantined" summary without re-walking the /// original `JUnit` input. #[must_use] -pub fn categorize(failing: Vec, verdict: &QuarantinedTests) -> QuarantineResult { +pub fn categorize( + failing: Vec, + quarantined_names: &BTreeSet, +) -> QuarantineResult { let mut quarantined = Vec::new(); let mut non_quarantined = Vec::new(); - let mut failing_not_quarantined_count = 0; for case in &failing { - let is_quarantined = verdict.quarantined.contains(&case.name); - if is_quarantined { + if quarantined_names.contains(&case.name) { quarantined.push(case.clone()); } else { - failing_not_quarantined_count += 1; - if verdict.non_quarantined.contains(&case.name) { - non_quarantined.push(case.clone()); - } + non_quarantined.push(case.clone()); } } @@ -154,7 +142,6 @@ pub fn categorize(failing: Vec, verdict: &QuarantinedTests) -> Quarant failing, quarantined, non_quarantined, - failing_not_quarantined_count, } } @@ -173,9 +160,8 @@ pub async fn check_failing( if failing.is_empty() { return Ok(QuarantineResult::default()); } - let names: Vec = failing.iter().map(|c| c.name.clone()).collect(); - let verdict = check(api_url, token, repository, branch, &names).await?; - Ok(categorize(failing, &verdict)) + let quarantined_names = fetch(api_url, token, repository, branch).await?; + Ok(categorize(failing, &quarantined_names)) } /// Lift a [`QuarantineFailed`] into the shared [`CliError`] so @@ -188,16 +174,9 @@ impl From for CliError { } } -#[derive(Serialize)] -struct CheckRequest<'a> { - tests_names: &'a [String], - branch: &'a str, -} - #[derive(Deserialize)] -struct CheckResponse { - quarantined_tests_names: Vec, - non_quarantined_tests_names: Vec, +struct QuarantinedTestName { + test_name: String, } #[cfg(test)] @@ -205,7 +184,7 @@ mod tests { use super::*; use crate::junit_process::junit::{Failure, TestStatus}; use std::time::Duration; - use wiremock::matchers::{body_json, header, method, path}; + use wiremock::matchers::{header, method, path, query_param, query_param_is_missing}; use wiremock::{Mock, MockServer, ResponseTemplate}; fn case(name: &str, status: TestStatus) -> TestCase { @@ -220,6 +199,24 @@ mod tests { } } + fn names(cases: &[TestCase]) -> Vec<&str> { + cases.iter().map(|c| c.name.as_str()).collect() + } + + fn page(test_names: &[&str]) -> serde_json::Value { + let tests: Vec<_> = test_names + .iter() + .map(|name| serde_json::json!({ "test_name": name })) + .collect(); + serde_json::json!({ "quarantined_tests": tests }) + } + + fn link(cursor: &str) -> String { + format!( + "; rel=\"next\"" + ) + } + #[test] fn categorize_buckets_quarantined_separately() { let failing = vec![ @@ -227,78 +224,99 @@ mod tests { case("b", TestStatus::Errored), case("c", TestStatus::Failed), ]; - let verdict = QuarantinedTests { - quarantined: ["a".to_string()].into_iter().collect(), - non_quarantined: ["b".to_string(), "c".to_string()].into_iter().collect(), - }; - let r = categorize(failing, &verdict); - assert_eq!( - r.quarantined.iter().map(|c| &c.name).collect::>(), - vec!["a"] - ); + let quarantined_names = ["a".to_string(), "unrelated".to_string()] + .into_iter() + .collect(); + let r = categorize(failing, &quarantined_names); + assert_eq!(names(&r.quarantined), vec!["a"]); + assert_eq!(names(&r.non_quarantined), vec!["b", "c"]); + // 2 failures not quarantined — drives the non-zero exit code. + assert_eq!(r.non_quarantined.len(), 2); + } + + #[tokio::test] + async fn fetch_follows_pagination_to_completion() { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/v1/ci/owner/repositories/repo/quarantines")) + .and(header("Authorization", "Bearer secret")) + .and(query_param("branch", "main")) + .and(query_param("per_page", "100")) + .and(query_param_is_missing("cursor")) + .respond_with( + ResponseTemplate::new(200) + .insert_header("link", link("c2").as_str()) + .set_body_json(page(&["t1"])), + ) + .expect(1) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/v1/ci/owner/repositories/repo/quarantines")) + .and(query_param("branch", "main")) + .and(query_param("per_page", "100")) + .and(query_param("cursor", "c2")) + .respond_with(ResponseTemplate::new(200).set_body_json(page(&["t2"]))) + .expect(1) + .mount(&server) + .await; + + let api_url = Url::parse(&server.uri()).unwrap(); + let quarantined = fetch(&api_url, "secret", "owner/repo", "main") + .await + .expect("API call succeeds"); assert_eq!( - r.non_quarantined - .iter() - .map(|c| &c.name) - .collect::>(), - vec!["b", "c"] + quarantined, + ["t1".to_string(), "t2".to_string()].into_iter().collect() ); - // 2 failures not quarantined — drives the non-zero exit code. - assert_eq!(r.failing_not_quarantined_count, 2); } - #[test] - fn categorize_counts_unknown_as_not_quarantined() { - // The API may omit names it doesn't recognize (e.g. typo, - // never seen before). Python treats those as failures that - // weren't quarantined → must count toward - // `failing_not_quarantined_count` even though they're not - // explicitly listed in `non_quarantined_tests_names`. - let failing = vec![case("x", TestStatus::Failed)]; - let verdict = QuarantinedTests::default(); - let r = categorize(failing, &verdict); - assert!(r.quarantined.is_empty()); - assert!(r.non_quarantined.is_empty()); - assert_eq!(r.failing_not_quarantined_count, 1); + #[tokio::test] + async fn fetch_surfaces_pagination_cycle_as_quarantine_failed() { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/v1/ci/owner/repositories/repo/quarantines")) + .respond_with( + ResponseTemplate::new(200) + .insert_header("link", link("loop").as_str()) + .set_body_json(page(&["t1"])), + ) + .expect(2) + .mount(&server) + .await; + + let api_url = Url::parse(&server.uri()).unwrap(); + let err = fetch(&api_url, "tok", "owner/repo", "main") + .await + .expect_err("a cycle must not return a partial list"); + assert_eq!( + err.message, + "quarantine pagination cycled back to a fetched page" + ); } #[tokio::test] - async fn check_posts_to_owner_scoped_path() { + async fn fetch_treats_payment_required_as_nothing_quarantined() { let server = MockServer::start().await; - Mock::given(method("POST")) - .and(path("/v1/ci/owner/repositories/repo/quarantines/check")) - .and(header("Authorization", "Bearer secret")) - .and(body_json(serde_json::json!({ - "tests_names": ["t1", "t2"], - "branch": "main", - }))) - .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ - "quarantined_tests_names": ["t1"], - "non_quarantined_tests_names": ["t2"], - }))) + Mock::given(method("GET")) + .and(path("/v1/ci/owner/repositories/repo/quarantines")) + .respond_with(ResponseTemplate::new(402).set_body_string("upgrade your plan")) + .expect(1) .mount(&server) .await; let api_url = Url::parse(&server.uri()).unwrap(); - let verdict = check( - &api_url, - "secret", - "owner/repo", - "main", - &["t1".to_string(), "t2".to_string()], - ) - .await - .expect("API call succeeds"); - assert_eq!(verdict.quarantined.len(), 1); - assert!(verdict.quarantined.contains("t1")); - assert!(verdict.non_quarantined.contains("t2")); + let quarantined = fetch(&api_url, "tok", "owner/repo", "main") + .await + .expect("402 is not a failure"); + assert!(quarantined.is_empty()); } #[tokio::test] async fn check_failing_short_circuits_when_no_failures() { // Empty failing list → no HTTP call, no QuarantineResult to - // categorize. Mirrors Python's early return. If the function - // accidentally tried to POST, the bogus URL would fail. + // categorize. If the function accidentally tried to fetch, + // the bogus URL would fail. let api_url = Url::parse("http://127.0.0.1:1").unwrap(); let cases = vec![ case("ok", TestStatus::Passed), @@ -308,18 +326,18 @@ mod tests { .await .expect("must short-circuit"); assert!(r.failing.is_empty()); - assert_eq!(r.failing_not_quarantined_count, 0); + assert!(r.non_quarantined.is_empty()); } #[tokio::test] - async fn check_surfaces_non_200_as_quarantine_failed() { + async fn fetch_surfaces_non_200_as_quarantine_failed() { let server = MockServer::start().await; - Mock::given(method("POST")) + Mock::given(method("GET")) .respond_with(ResponseTemplate::new(503).set_body_string("backend down")) .mount(&server) .await; let api_url = Url::parse(&server.uri()).unwrap(); - let err = check(&api_url, "tok", "owner/repo", "main", &["t".to_string()]) + let err = fetch(&api_url, "tok", "owner/repo", "main") .await .expect_err("503 must surface as QuarantineFailed"); assert!( diff --git a/crates/mergify-ci/src/tests_quarantine.rs b/crates/mergify-ci/src/tests_quarantine.rs index fd85ee1b..b2c1defa 100644 --- a/crates/mergify-ci/src/tests_quarantine.rs +++ b/crates/mergify-ci/src/tests_quarantine.rs @@ -274,8 +274,8 @@ struct AddQuarantineResponse { /// id with the minimal [`QuarantineListItem`], while `quarantined` /// reads the full [`QuarantinedTest`] for display. #[derive(Deserialize)] -struct QuarantineList { - quarantined_tests: Vec, +pub(crate) struct QuarantineList { + pub(crate) quarantined_tests: Vec, } #[derive(Deserialize)] diff --git a/crates/mergify-core/src/http.rs b/crates/mergify-core/src/http.rs index 94a7d226..06fbe455 100644 --- a/crates/mergify-core/src/http.rs +++ b/crates/mergify-core/src/http.rs @@ -107,8 +107,9 @@ pub struct Page { enum OnTerminalError { /// Render it into a [`CliError`]. The default. Fail, - /// 404 short-circuits to `Ok(None)`; everything else fails. - NotFoundIsNone, + /// The given status short-circuits to `Ok(None)`; everything + /// else fails. + StatusIsNone(u16), /// Hand the response back unread so the caller can decode a /// protocol-defined error body. ReturnResponse, @@ -245,18 +246,50 @@ impl Client { path: &str, query: &[(&str, &str)], ) -> Result, CliError> { + // `OnTerminalError::Fail` never returns `None`; + // `Option::expect` documents that invariant. + Ok(self + .fetch_page(path, query, OnTerminalError::Fail) + .await? + .expect("fetch_page returned None despite OnTerminalError::Fail")) + } + + /// [`Self::get_page`], but return `None` when the server answers + /// `status` — for an endpoint where that status is a routine + /// caller branch (e.g. 402: the feature is not in the plan). + pub async fn get_page_unless( + &self, + path: &str, + query: &[(&str, &str)], + status: u16, + ) -> Result>, CliError> { + self.fetch_page(path, query, OnTerminalError::StatusIsNone(status)) + .await + } + + async fn fetch_page( + &self, + path: &str, + query: &[(&str, &str)], + terminal: OnTerminalError, + ) -> Result>, CliError> { let mut url = self.join(path)?; if !query.is_empty() { url.query_pairs_mut().extend_pairs(query.iter().copied()); } - let resp = self.execute_request(self.inner.get(url)).await?; + let Some(resp) = self + .execute_with_retry(self.inner.get(url), terminal, None) + .await? + else { + return Ok(None); + }; let next_cursor = resp .headers() .get(reqwest::header::LINK) .and_then(|value| value.to_str().ok()) .and_then(next_cursor_from_link); let body = self.decode_json(resp).await?; - Ok(Page { body, next_cursor }) + Ok(Some(Page { body, next_cursor })) } /// GET `path`, decoding the JSON body as `T` on success and as @@ -438,9 +471,13 @@ impl Client { /// as taking no body should be sent none, not a JSON `null`. pub async fn post_empty_if_exists(&self, path: &str) -> Result<(), CliError> { let url = self.join(path)?; - self.execute_with_retry(self.inner.post(url), OnTerminalError::NotFoundIsNone, None) - .await - .map(drop) + self.execute_with_retry( + self.inner.post(url), + OnTerminalError::StatusIsNone(StatusCode::NOT_FOUND.as_u16()), + None, + ) + .await + .map(drop) } /// PUT `body` as JSON to `path` and deserialize the JSON @@ -603,11 +640,6 @@ impl Client { if status.is_success() { return Ok(Some(resp)); } - if terminal == OnTerminalError::NotFoundIsNone - && status == StatusCode::NOT_FOUND - { - return Ok(None); - } // Inspect rate-limit headers before the body is // read. GitHub signals secondary/abuse limits with // 429, or 403 carrying `Retry-After` / an exhausted @@ -618,6 +650,13 @@ impl Client { || (status == StatusCode::FORBIDDEN && rate_limit.is_some()); let retryable = (status.is_server_error() || rate_limited) && attempt + 1 < self.retry.max_attempts; + let protocol_answer = !retryable && !status.is_server_error() && !rate_limited; + // After the retry decision: a server failure or a + // throttle is never a routine absence. + if protocol_answer && terminal == OnTerminalError::StatusIsNone(status.as_u16()) + { + return Ok(None); + } // Terminal, and the caller wants the body: hand the // response over unread. Reading it here to render a // message would consume the very bytes the caller @@ -640,11 +679,7 @@ impl Client { // "the credential was refused" would tell the user // their credential is revoked — `auth status` // does exactly that. - if !retryable - && !status.is_server_error() - && !rate_limited - && terminal == OnTerminalError::ReturnResponse - { + if protocol_answer && terminal == OnTerminalError::ReturnResponse { return Ok(Some(resp)); } last_message = error_message(status, resp).await; @@ -719,8 +754,12 @@ impl Client { &self, builder: reqwest::RequestBuilder, ) -> Result, CliError> { - self.execute_with_retry(builder, OnTerminalError::NotFoundIsNone, None) - .await + self.execute_with_retry( + builder, + OnTerminalError::StatusIsNone(StatusCode::NOT_FOUND.as_u16()), + None, + ) + .await } /// Send a request that cares only about the HTTP status. @@ -731,7 +770,11 @@ impl Client { builder: reqwest::RequestBuilder, ) -> Result { match self - .execute_with_retry(builder, OnTerminalError::NotFoundIsNone, None) + .execute_with_retry( + builder, + OnTerminalError::StatusIsNone(StatusCode::NOT_FOUND.as_u16()), + None, + ) .await? { Some(_) => Ok(DeleteOutcome::Deleted), @@ -2039,6 +2082,42 @@ mod tests { assert_eq!(page.next_cursor, None); } + #[tokio::test] + async fn get_page_unless_returns_none_on_the_given_status() { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/paged")) + .respond_with(ResponseTemplate::new(402)) + .expect(1) + .mount(&server) + .await; + + let client = fast_client(&server, ApiFlavor::Mergify); + let page: Option> = client.get_page_unless("/paged", &[], 402).await.unwrap(); + assert!(page.is_none()); + } + + #[tokio::test] + async fn get_page_unless_still_retries_and_fails_a_server_error() { + // A 5xx is the server failing, never a routine absence, even + // when the caller names it. + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/paged")) + .respond_with(ResponseTemplate::new(503)) + .expect(3) + .mount(&server) + .await; + + let client = fast_client(&server, ApiFlavor::Mergify); + let err = client + .get_page_unless::("/paged", &[], 503) + .await + .err() + .expect("a 5xx must fail"); + assert!(matches!(err, CliError::MergifyApi(_))); + } + #[tokio::test] async fn get_if_exists_returns_some_on_2xx() { let server = MockServer::start().await;