From bb543bde0eec7cf12ed3e1cb07087b86a2b91ddc Mon Sep 17 00:00:00 2001 From: max-trunk Date: Fri, 24 Jul 2026 16:11:17 +0000 Subject: [PATCH 1/5] feat(quarantine): send test collection id and record resolution mode Pipe the existing --test-collection-id value into the getQuarantineConfig request (testCollectionShortId) and record the response's new quarantineResolutionMode field in telemetry, across both the traditional and rspec invocation paths. - api: add test_collection_short_id to GetQuarantineConfigRequest and quarantine_resolution_mode (Option, serde default) to the response so older servers that omit it still deserialize. - populate test_collection_short_id from meta (traditional) and from TRUNK_TEST_COLLECTION_ID (rspec). - proto: add QuarantineResolutionMode enum and UploadMetrics field 9; carry the mode through QuarantineContext and a RunUploadOptions override. - emit a Sentry-only tracing event (hidden_in_console) with the resolved mode in both paths for debugging. Server side: trunk-io/trunk#33132 Co-Authored-By: Claude Opus 4.8 (1M context) --- api/src/client.rs | 4 ++ api/src/message.rs | 63 ++++++++++++++++++++++++++++++++ cli/src/context.rs | 6 +++ cli/src/context_quarantine.rs | 35 +++++++++++++++++- cli/src/upload_command.rs | 11 +++++- cli/tests/test.rs | 3 ++ cli/tests/upload.rs | 6 +++ proto/proto/upload_metrics.proto | 12 ++++++ test_report/src/report.rs | 30 +++++++++++++++ test_report/tests/quarantine.rs | 3 ++ test_report/tests/telemetry.rs | 1 + test_utils/src/mock_server.rs | 1 + 12 files changed, 172 insertions(+), 3 deletions(-) diff --git a/api/src/client.rs b/api/src/client.rs index 90d0d059..226d07cb 100644 --- a/api/src/client.rs +++ b/api/src/client.rs @@ -537,6 +537,7 @@ mod tests { org_url_slug: String::from("org_url_slug"), test_identifiers: vec![], remote_urls: vec![], + test_collection_short_id: None, }) .await .unwrap_err() @@ -585,6 +586,7 @@ mod tests { remote_urls: vec![], org_url_slug: String::from("org_url_slug"), test_identifiers: vec![], + test_collection_short_id: None, }) .await .unwrap_err() @@ -628,6 +630,7 @@ mod tests { remote_urls: vec![], org_url_slug: String::from("org_url_slug"), test_identifiers: vec![], + test_collection_short_id: None, }) .await .unwrap_err() @@ -709,6 +712,7 @@ mod tests { org_url_slug: String::from("org_url_slug"), test_identifiers: vec![], remote_urls: vec![], + test_collection_short_id: None, }) .await; diff --git a/api/src/message.rs b/api/src/message.rs index 1e372841..2bd46238 100644 --- a/api/src/message.rs +++ b/api/src/message.rs @@ -30,6 +30,10 @@ pub struct GetQuarantineConfigResponse { pub is_disabled: bool, #[serde(rename = "testIds")] pub quarantined_tests: Vec, + /// Which source the server used to resolve quarantine status: "repo" or + /// "test_collection". Absent on older servers, hence `Option` + `default`. + #[serde(default)] + pub quarantine_resolution_mode: Option, } #[derive(Debug, Serialize, Clone, Deserialize, PartialEq, Eq)] @@ -39,6 +43,10 @@ pub struct GetQuarantineConfigRequest { pub remote_urls: Vec, pub org_url_slug: String, pub test_identifiers: Vec, + /// Optional test collection short id (from `--test-collection-id`). When set and the org is + /// migrated to test collections, the server resolves quarantine status from the collection. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub test_collection_short_id: Option, } #[derive(Debug, Serialize, Clone, Deserialize, PartialEq, Eq)] @@ -61,3 +69,58 @@ pub struct CreateBundleUploadIntentResponse { pub struct TelemetryUploadMetricsRequest { pub upload_metrics: proto::upload_metrics::trunk::UploadMetrics, } + +#[cfg(test)] +mod tests { + use super::*; + + fn base_request() -> GetQuarantineConfigRequest { + GetQuarantineConfigRequest { + repo: RepoUrlParts { + host: String::from("github.com"), + owner: String::from("trunk-io"), + name: String::from("analytics-cli"), + }, + remote_urls: vec![], + org_url_slug: String::from("trunk"), + test_identifiers: vec![], + test_collection_short_id: None, + } + } + + #[test] + fn serializes_test_collection_short_id_as_camel_case_when_set() { + let request = GetQuarantineConfigRequest { + test_collection_short_id: Some(String::from("abcd1234")), + ..base_request() + }; + let value: serde_json::Value = serde_json::to_value(&request).unwrap(); + assert_eq!(value["testCollectionShortId"], "abcd1234"); + } + + #[test] + fn omits_test_collection_short_id_when_absent() { + let value: serde_json::Value = serde_json::to_value(base_request()).unwrap(); + assert!(value.get("testCollectionShortId").is_none()); + } + + #[test] + fn deserializes_response_with_quarantine_resolution_mode() { + let response: GetQuarantineConfigResponse = serde_json::from_str( + r#"{ "isDisabled": false, "testIds": ["id1"], "quarantineResolutionMode": "test_collection" }"#, + ) + .unwrap(); + assert_eq!( + response.quarantine_resolution_mode.as_deref(), + Some("test_collection") + ); + } + + #[test] + fn deserializes_response_without_quarantine_resolution_mode() { + // Older servers omit the field; deserialization must still succeed. + let response: GetQuarantineConfigResponse = + serde_json::from_str(r#"{ "isDisabled": false, "testIds": [] }"#).unwrap(); + assert_eq!(response.quarantine_resolution_mode, None); + } +} diff --git a/cli/src/context.rs b/cli/src/context.rs index f4a9ce42..1a819406 100644 --- a/cli/src/context.rs +++ b/cli/src/context.rs @@ -801,6 +801,7 @@ pub async fn gather_exit_code_and_quarantined_tests_context( repo: RepoUrlParts::default(), org_url_slug: String::default(), fetch_status: QuarantineFetchStatus::FetchSkipped, + quarantine_resolution_mode: None, } } else { // default to success if no test run result (i.e. `upload`) @@ -814,6 +815,11 @@ pub async fn gather_exit_code_and_quarantined_tests_context( org_url_slug: meta.base_props.org.clone(), test_identifiers: failed_tests_extractor.failed_tests().to_vec(), remote_urls: vec![meta.base_props.repo.repo_url.clone()], + test_collection_short_id: meta + .base_props + .test_collection + .as_ref() + .map(|tc| tc.short_id.clone()), }, file_set_builder, Some(failed_tests_extractor), diff --git a/cli/src/context_quarantine.rs b/cli/src/context_quarantine.rs index 7146f4ce..a22f2adf 100644 --- a/cli/src/context_quarantine.rs +++ b/cli/src/context_quarantine.rs @@ -21,7 +21,7 @@ use context::{ }; use pluralizer::pluralize; use prost::Message; -use proto::upload_metrics::trunk::QuarantineQueryResult; +use proto::upload_metrics::trunk::{QuarantineQueryResult, QuarantineResolutionMode}; #[derive(Debug)] pub enum QuarantineFetchStatus { @@ -47,6 +47,10 @@ pub struct QuarantineContext { pub repo: RepoUrlParts, pub org_url_slug: String, pub fetch_status: QuarantineFetchStatus, + /// Which source the server used to resolve quarantine status ("repo" or + /// "test_collection"), from the getQuarantineConfig response. `None` when no + /// fetch ran or the server omitted the field. + pub quarantine_resolution_mode: Option, } impl QuarantineContext { pub fn skip_fetch(failures: Vec) -> Self { @@ -57,6 +61,7 @@ impl QuarantineContext { repo: RepoUrlParts::default(), org_url_slug: String::default(), fetch_status: QuarantineFetchStatus::FetchSkipped, + quarantine_resolution_mode: None, } } @@ -68,6 +73,7 @@ impl QuarantineContext { repo: RepoUrlParts::default(), org_url_slug: String::default(), fetch_status: QuarantineFetchStatus::FetchFailed(error), + quarantine_resolution_mode: None, } } } @@ -87,6 +93,14 @@ pub fn quarantine_query_result( } } +pub fn quarantine_resolution_mode(ctx: &QuarantineContext) -> QuarantineResolutionMode { + match ctx.quarantine_resolution_mode.as_deref() { + Some("repo") => QuarantineResolutionMode::Repo, + Some("test_collection") => QuarantineResolutionMode::TestCollection, + _ => QuarantineResolutionMode::Unspecified, + } +} + fn convert_case_to_test + ToString>( repo: &RepoUrlParts, org_slug: T, @@ -322,6 +336,7 @@ pub async fn gather_quarantine_context( quarantine_status: QuarantineBulkTestStatus::default(), failures: Vec::default(), fetch_status: QuarantineFetchStatus::FetchSkipped, + quarantine_resolution_mode: None, }); } @@ -336,7 +351,17 @@ pub async fn gather_quarantine_context( { tracing::info!("Checking if failed tests can be quarantined"); match api_client.get_quarantining_config(request).await { - anyhow::Result::Ok(response) => (Some(response), QuarantineFetchStatus::FetchSucceeded), + anyhow::Result::Ok(response) => { + // Surface which source the server resolved quarantine status from. Sent to Sentry + // only (hidden from the console) to aid debugging without adding CLI noise. + tracing::info!( + hidden_in_console = true, + quarantine_resolution_mode = + response.quarantine_resolution_mode.as_deref().unwrap_or("unspecified"), + "Resolved quarantine config" + ); + (Some(response), QuarantineFetchStatus::FetchSucceeded) + } anyhow::Result::Err(error) => (None, QuarantineFetchStatus::FetchFailed(error)), } } else { @@ -344,6 +369,10 @@ pub async fn gather_quarantine_context( (None, QuarantineFetchStatus::FetchSkipped) }; + let quarantine_resolution_mode = quarantine_config + .as_ref() + .and_then(|config| config.quarantine_resolution_mode.clone()); + // if quarantining is not enabled, return exit code and empty quarantine status if quarantine_config .as_ref() @@ -361,6 +390,7 @@ pub async fn gather_quarantine_context( repo: request.repo.clone(), org_url_slug: request.org_url_slug.clone(), fetch_status: quarantine_fetch_status, + quarantine_resolution_mode, }); } else { // quarantining is enabled, continue with quarantine process and update exit code @@ -447,6 +477,7 @@ pub async fn gather_quarantine_context( repo: request.repo.clone(), org_url_slug: request.org_url_slug.clone(), fetch_status: quarantine_fetch_status, + quarantine_resolution_mode, }) } diff --git a/cli/src/upload_command.rs b/cli/src/upload_command.rs index e95f5707..e7367256 100644 --- a/cli/src/upload_command.rs +++ b/cli/src/upload_command.rs @@ -18,7 +18,9 @@ use superconsole::{ }; use tempfile::TempDir; -use crate::context_quarantine::{QuarantineContext, quarantine_query_result}; +use crate::context_quarantine::{ + QuarantineContext, quarantine_query_result, quarantine_resolution_mode, +}; use crate::validate_command::JunitReportValidations; use crate::{ context::{ @@ -380,6 +382,8 @@ pub struct RunUploadOptions { pub render_sender: Option>, pub quarantine_query_result_override: Option, + pub quarantine_resolution_mode_override: + Option, } impl Default for RunUploadOptions { @@ -389,6 +393,7 @@ impl Default for RunUploadOptions { test_run_result: None, render_sender: None, quarantine_query_result_override: None, + quarantine_resolution_mode_override: None, } } } @@ -402,6 +407,7 @@ pub async fn run_upload( test_run_result, render_sender, quarantine_query_result_override, + quarantine_resolution_mode_override, } = options; // grab the exec start if provided (`test` subcommand) or use the current time let cli_started_at = if let Some(test_run_result) = test_run_result.as_ref() { @@ -562,6 +568,8 @@ pub async fn run_upload( .await; let quarantine_query_result = quarantine_query_result_override .unwrap_or_else(|| quarantine_query_result(disable_quarantining, &quarantine_context)); + let quarantine_resolution_mode = quarantine_resolution_mode_override + .unwrap_or_else(|| quarantine_resolution_mode(&quarantine_context)); let upload_metrics = proto::upload_metrics::trunk::UploadMetrics { client_version: Some(proto::upload_metrics::trunk::Semver { major: env!("CARGO_PKG_VERSION_MAJOR").parse().unwrap_or_default(), @@ -580,6 +588,7 @@ pub async fn run_upload( failed: false, failure_reason: "".into(), quarantine_query_result: quarantine_query_result.into(), + quarantine_resolution_mode: quarantine_resolution_mode.into(), }; let mut request = api::message::TelemetryUploadMetricsRequest { upload_metrics }; if !upload_args.dry_run { diff --git a/cli/tests/test.rs b/cli/tests/test.rs index a789541e..7d163b80 100644 --- a/cli/tests/test.rs +++ b/cli/tests/test.rs @@ -287,6 +287,7 @@ async fn quarantining_resets_fail_code() { Json(GetQuarantineConfigResponse { is_disabled: false, quarantined_tests: test_ids, + ..Default::default() }) } }, @@ -347,6 +348,7 @@ async fn quarantining_not_active_when_disable_quarantining_set() { Json(GetQuarantineConfigResponse { is_disabled: false, quarantined_tests: test_ids, + ..Default::default() }) } }, @@ -408,6 +410,7 @@ async fn quarantining_not_active_when_disable_true_but_use_true() { Json(GetQuarantineConfigResponse { is_disabled: false, quarantined_tests: test_ids, + ..Default::default() }) } }, diff --git a/cli/tests/upload.rs b/cli/tests/upload.rs index 5f83445a..53e67e45 100644 --- a/cli/tests/upload.rs +++ b/cli/tests/upload.rs @@ -1294,6 +1294,7 @@ async fn quarantines_tests_regardless_of_upload() { Json(GetQuarantineConfigResponse { is_disabled, quarantined_tests, + ..Default::default() }) } }, @@ -2041,6 +2042,7 @@ async fn do_not_quarantines_tests_when_quarantine_disabled_set() { Json(GetQuarantineConfigResponse { is_disabled, quarantined_tests, + ..Default::default() }) } }, @@ -2149,6 +2151,7 @@ async fn still_quarantines_if_upload_to_s3_fails() { Json(GetQuarantineConfigResponse { is_disabled: false, quarantined_tests: test_ids, + ..Default::default() }) } }, @@ -2189,6 +2192,7 @@ async fn still_quarantines_if_upload_endpoint_fails() { Json(GetQuarantineConfigResponse { is_disabled: false, quarantined_tests: test_ids, + ..Default::default() }) } }, @@ -2229,6 +2233,7 @@ async fn presents_error_message_if_you_dont_have_permission_for_s3_upload() { Json(GetQuarantineConfigResponse { is_disabled: false, quarantined_tests: test_ids, + ..Default::default() }) } }, @@ -2270,6 +2275,7 @@ async fn presents_error_message_if_you_dont_have_permission_for_upload_endpoint( Json(GetQuarantineConfigResponse { is_disabled: false, quarantined_tests: test_ids, + ..Default::default() }) } }, diff --git a/proto/proto/upload_metrics.proto b/proto/proto/upload_metrics.proto index 68e1999b..47d459ec 100644 --- a/proto/proto/upload_metrics.proto +++ b/proto/proto/upload_metrics.proto @@ -20,6 +20,17 @@ enum QuarantineQueryResult { QUARANTINE_QUERY_RESULT_CACHED = 5; } +// Which source the server used to resolve quarantine status, from the +// getQuarantineConfig response's `quarantineResolutionMode` field. +enum QuarantineResolutionMode { + // Default; the response omitted the field (older server) or no lookup ran. + QUARANTINE_RESOLUTION_MODE_UNSPECIFIED = 0; + // Quarantine status was resolved from the repo. + QUARANTINE_RESOLUTION_MODE_REPO = 1; + // Quarantine status was resolved from the test collection. + QUARANTINE_RESOLUTION_MODE_TEST_COLLECTION = 2; +} + message UploadMetrics { Semver client_version = 1; Repo repo = 2; @@ -29,6 +40,7 @@ message UploadMetrics { bool failed = 6; string failure_reason = 7; QuarantineQueryResult quarantine_query_result = 8; + QuarantineResolutionMode quarantine_resolution_mode = 9; } // Used by the analytics-uploader, kept here to avoid collisions in the future. diff --git a/test_report/src/report.rs b/test_report/src/report.rs index 71303a9e..8fe47fb7 100644 --- a/test_report/src/report.rs +++ b/test_report/src/report.rs @@ -65,6 +65,9 @@ pub struct TestReport { started_at: SystemTime, quarantine_config: Option, quarantine_lookup_source: Option, + /// Which source the server resolved quarantine status from ("repo" or + /// "test_collection"), captured from the getQuarantineConfig response. + quarantine_resolution_mode: Option, quarantined_tests_disk_cache_ttl: Duration, codeowners: Option, variant: Option, @@ -239,6 +242,7 @@ impl MutTestReport { started_at, quarantine_config: None, quarantine_lookup_source: None, + quarantine_resolution_mode: None, quarantined_tests_disk_cache_ttl, codeowners, repo, @@ -264,6 +268,18 @@ impl MutTestReport { } } + fn quarantine_resolution_mode_for_telemetry( + &self, + ) -> proto::upload_metrics::trunk::QuarantineResolutionMode { + use proto::upload_metrics::trunk::QuarantineResolutionMode; + + match self.0.borrow().quarantine_resolution_mode.as_deref() { + Some("repo") => QuarantineResolutionMode::Repo, + Some("test_collection") => QuarantineResolutionMode::TestCollection, + _ => QuarantineResolutionMode::Unspecified, + } + } + fn serialize_test_report(&self) -> Vec { prost::Message::encode_to_vec(&self.0.borrow().test_report) } @@ -539,6 +555,7 @@ impl MutTestReport { test_identifiers: vec![], remote_urls: vec![repo_url.clone()], repo: repo.clone(), + test_collection_short_id: env::var(constants::TRUNK_TEST_COLLECTION_ID_ENV).ok(), }; let response = tokio::runtime::Builder::new_multi_thread() .enable_all() @@ -551,6 +568,16 @@ impl MutTestReport { for quarantined_test_id in response.quarantined_tests.iter() { quarantined_tests.insert(quarantined_test_id.clone(), true); } + // Surface which source the server resolved quarantine status from. Sent to Sentry + // only (hidden from the console) to aid debugging without adding CLI noise. + tracing::info!( + hidden_in_console = true, + quarantine_resolution_mode = + response.quarantine_resolution_mode.as_deref().unwrap_or("unspecified"), + "Resolved quarantine config" + ); + self.0.borrow_mut().quarantine_resolution_mode = + response.quarantine_resolution_mode.clone(); if is_disabled { self.record_quarantine_lookup_source(QuarantineLookupSource::Disabled); } else { @@ -724,6 +751,9 @@ impl MutTestReport { quarantine_query_result_override: Some( self.quarantine_query_result_for_telemetry(), ), + quarantine_resolution_mode_override: Some( + self.quarantine_resolution_mode_for_telemetry(), + ), ..Default::default() }, )) { diff --git a/test_report/tests/quarantine.rs b/test_report/tests/quarantine.rs index ee5f148f..7ecb83e9 100644 --- a/test_report/tests/quarantine.rs +++ b/test_report/tests/quarantine.rs @@ -118,6 +118,7 @@ async fn quarantine_variant_impacts_quarantining() { Json(GetQuarantineConfigResponse { is_disabled: false, quarantined_tests: vec![expected_id_v1, expected_id_v1_from_base], + ..Default::default() }) }, ); @@ -402,6 +403,7 @@ async fn quarantine_disk_cache() { computed_test_id_1_clone.clone(), computed_test_id_2_clone.clone(), ], + ..Default::default() }) }, ); @@ -563,6 +565,7 @@ async fn quarantine_disabled_for_repo() { Json(GetQuarantineConfigResponse { is_disabled: true, quarantined_tests: vec![], + ..Default::default() }) }, ); diff --git a/test_report/tests/telemetry.rs b/test_report/tests/telemetry.rs index d0cd6c68..0958b157 100644 --- a/test_report/tests/telemetry.rs +++ b/test_report/tests/telemetry.rs @@ -205,6 +205,7 @@ async fn telemetry_query_result_disabled_on_publish() { Json(GetQuarantineConfigResponse { is_disabled: true, quarantined_tests: vec![], + ..Default::default() }) }, ); diff --git a/test_utils/src/mock_server.rs b/test_utils/src/mock_server.rs index 19f742d3..ee014bb8 100644 --- a/test_utils/src/mock_server.rs +++ b/test_utils/src/mock_server.rs @@ -204,6 +204,7 @@ pub async fn get_quarantining_config_handler( Json(GetQuarantineConfigResponse { is_disabled: false, quarantined_tests: Vec::new(), + ..Default::default() }) } From 27b988a36b9a5f06fa732c25e24ea90d6a37797d Mon Sep 17 00:00:00 2001 From: max-trunk Date: Fri, 24 Jul 2026 20:37:41 +0000 Subject: [PATCH 2/5] refactor(quarantine): address review feedback on resolution mode Introduce a typed api::message::QuarantineResolutionMode enum with a tolerant deserializer (unknown or absent values -> Unspecified) so a new server-side mode never breaks deserialization on older clients. It replaces the Option previously threaded through the response, QuarantineContext, TestReport, and the quarantine disk cache, and collapses the two duplicated string-match sites into a single From impl. Also: - Cache the resolution mode in the disk-cache entry and restore it on a cache hit, so telemetry reports the true mode without a re-fetch. - Rework resolution-mode logging into human-readable messages, emitted at most once per report. - Drop the redundant standalone TestReport field (telemetry now reads from quarantine_config). - Populate quarantine_resolution_mode in the QuarantineContext test constructors the vendored-openssl build never compiled. Co-Authored-By: Claude Opus 4.8 --- api/src/message.rs | 65 ++++++++++++++++++++++++++++++---- cli/src/context.rs | 2 +- cli/src/context_quarantine.rs | 49 +++++++++++++------------- test_report/src/report.rs | 66 +++++++++++++++++++++++------------ 4 files changed, 128 insertions(+), 54 deletions(-) diff --git a/api/src/message.rs b/api/src/message.rs index 2bd46238..2a017f09 100644 --- a/api/src/message.rs +++ b/api/src/message.rs @@ -24,16 +24,52 @@ pub struct CreateBundleUploadResponse { pub test_collection_bundle_meta_created_at: Option, } +/// Which source the server resolved quarantine status from, mapped from the +/// getQuarantineConfig response's `quarantineResolutionMode` string. Unknown or +/// absent values deserialize to `Unspecified` so a new server-side value never +/// breaks deserialization on older clients. +#[derive(Debug, Serialize, Clone, Copy, PartialEq, Eq, Default)] +#[serde(rename_all = "snake_case")] +pub enum QuarantineResolutionMode { + Repo, + TestCollection, + #[default] + Unspecified, +} + +impl<'de> Deserialize<'de> for QuarantineResolutionMode { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + Ok( + match Option::::deserialize(deserializer)?.as_deref() { + Some("repo") => Self::Repo, + Some("test_collection") => Self::TestCollection, + _ => Self::Unspecified, + }, + ) + } +} + +impl From for proto::upload_metrics::trunk::QuarantineResolutionMode { + fn from(mode: QuarantineResolutionMode) -> Self { + match mode { + QuarantineResolutionMode::Repo => Self::Repo, + QuarantineResolutionMode::TestCollection => Self::TestCollection, + QuarantineResolutionMode::Unspecified => Self::Unspecified, + } + } +} + #[derive(Debug, Serialize, Clone, Deserialize, Default)] #[serde(rename_all = "camelCase")] pub struct GetQuarantineConfigResponse { pub is_disabled: bool, #[serde(rename = "testIds")] pub quarantined_tests: Vec, - /// Which source the server used to resolve quarantine status: "repo" or - /// "test_collection". Absent on older servers, hence `Option` + `default`. #[serde(default)] - pub quarantine_resolution_mode: Option, + pub quarantine_resolution_mode: QuarantineResolutionMode, } #[derive(Debug, Serialize, Clone, Deserialize, PartialEq, Eq)] @@ -111,8 +147,8 @@ mod tests { ) .unwrap(); assert_eq!( - response.quarantine_resolution_mode.as_deref(), - Some("test_collection") + response.quarantine_resolution_mode, + QuarantineResolutionMode::TestCollection ); } @@ -121,6 +157,23 @@ mod tests { // Older servers omit the field; deserialization must still succeed. let response: GetQuarantineConfigResponse = serde_json::from_str(r#"{ "isDisabled": false, "testIds": [] }"#).unwrap(); - assert_eq!(response.quarantine_resolution_mode, None); + assert_eq!( + response.quarantine_resolution_mode, + QuarantineResolutionMode::Unspecified + ); + } + + #[test] + fn deserializes_unknown_quarantine_resolution_mode_as_unspecified() { + // A resolution mode the client doesn't recognize must not fail the whole + // response parse; it degrades to Unspecified. + let response: GetQuarantineConfigResponse = serde_json::from_str( + r#"{ "isDisabled": false, "testIds": [], "quarantineResolutionMode": "something_new" }"#, + ) + .unwrap(); + assert_eq!( + response.quarantine_resolution_mode, + QuarantineResolutionMode::Unspecified + ); } } diff --git a/cli/src/context.rs b/cli/src/context.rs index 1a819406..2f8bf97e 100644 --- a/cli/src/context.rs +++ b/cli/src/context.rs @@ -801,7 +801,7 @@ pub async fn gather_exit_code_and_quarantined_tests_context( repo: RepoUrlParts::default(), org_url_slug: String::default(), fetch_status: QuarantineFetchStatus::FetchSkipped, - quarantine_resolution_mode: None, + quarantine_resolution_mode: api::message::QuarantineResolutionMode::Unspecified, } } else { // default to success if no test run result (i.e. `upload`) diff --git a/cli/src/context_quarantine.rs b/cli/src/context_quarantine.rs index a22f2adf..f3bdb52b 100644 --- a/cli/src/context_quarantine.rs +++ b/cli/src/context_quarantine.rs @@ -3,7 +3,7 @@ use std::{ io::{BufReader, Read}, }; -use api::{client::ApiClient, urls::url_for_test_case}; +use api::{client::ApiClient, message::QuarantineResolutionMode, urls::url_for_test_case}; use bundle::{ FileSet, FileSetBuilder, FileSetTestRunnerReport, FileSetType, QuarantineBulkTestStatus, Test, }; @@ -21,7 +21,7 @@ use context::{ }; use pluralizer::pluralize; use prost::Message; -use proto::upload_metrics::trunk::{QuarantineQueryResult, QuarantineResolutionMode}; +use proto::upload_metrics::trunk::QuarantineQueryResult; #[derive(Debug)] pub enum QuarantineFetchStatus { @@ -47,10 +47,7 @@ pub struct QuarantineContext { pub repo: RepoUrlParts, pub org_url_slug: String, pub fetch_status: QuarantineFetchStatus, - /// Which source the server used to resolve quarantine status ("repo" or - /// "test_collection"), from the getQuarantineConfig response. `None` when no - /// fetch ran or the server omitted the field. - pub quarantine_resolution_mode: Option, + pub quarantine_resolution_mode: api::message::QuarantineResolutionMode, } impl QuarantineContext { pub fn skip_fetch(failures: Vec) -> Self { @@ -61,7 +58,7 @@ impl QuarantineContext { repo: RepoUrlParts::default(), org_url_slug: String::default(), fetch_status: QuarantineFetchStatus::FetchSkipped, - quarantine_resolution_mode: None, + quarantine_resolution_mode: api::message::QuarantineResolutionMode::Unspecified, } } @@ -73,7 +70,7 @@ impl QuarantineContext { repo: RepoUrlParts::default(), org_url_slug: String::default(), fetch_status: QuarantineFetchStatus::FetchFailed(error), - quarantine_resolution_mode: None, + quarantine_resolution_mode: api::message::QuarantineResolutionMode::Unspecified, } } } @@ -93,12 +90,10 @@ pub fn quarantine_query_result( } } -pub fn quarantine_resolution_mode(ctx: &QuarantineContext) -> QuarantineResolutionMode { - match ctx.quarantine_resolution_mode.as_deref() { - Some("repo") => QuarantineResolutionMode::Repo, - Some("test_collection") => QuarantineResolutionMode::TestCollection, - _ => QuarantineResolutionMode::Unspecified, - } +pub fn quarantine_resolution_mode( + ctx: &QuarantineContext, +) -> proto::upload_metrics::trunk::QuarantineResolutionMode { + ctx.quarantine_resolution_mode.into() } fn convert_case_to_test + ToString>( @@ -336,7 +331,7 @@ pub async fn gather_quarantine_context( quarantine_status: QuarantineBulkTestStatus::default(), failures: Vec::default(), fetch_status: QuarantineFetchStatus::FetchSkipped, - quarantine_resolution_mode: None, + quarantine_resolution_mode: api::message::QuarantineResolutionMode::Unspecified, }); } @@ -352,14 +347,17 @@ pub async fn gather_quarantine_context( tracing::info!("Checking if failed tests can be quarantined"); match api_client.get_quarantining_config(request).await { anyhow::Result::Ok(response) => { - // Surface which source the server resolved quarantine status from. Sent to Sentry - // only (hidden from the console) to aid debugging without adding CLI noise. - tracing::info!( - hidden_in_console = true, - quarantine_resolution_mode = - response.quarantine_resolution_mode.as_deref().unwrap_or("unspecified"), - "Resolved quarantine config" - ); + match response.quarantine_resolution_mode { + QuarantineResolutionMode::TestCollection => tracing::info!( + "Resolved quarantine status for test collection {}", + request.test_collection_short_id.as_deref().unwrap_or("unknown"), + ), + QuarantineResolutionMode::Repo => tracing::info!( + "Resolved quarantine status for repo {}", + request.repo.repo_full_name(), + ), + QuarantineResolutionMode::Unspecified => {} + } (Some(response), QuarantineFetchStatus::FetchSucceeded) } anyhow::Result::Err(error) => (None, QuarantineFetchStatus::FetchFailed(error)), @@ -371,7 +369,8 @@ pub async fn gather_quarantine_context( let quarantine_resolution_mode = quarantine_config .as_ref() - .and_then(|config| config.quarantine_resolution_mode.clone()); + .map(|config| config.quarantine_resolution_mode) + .unwrap_or_default(); // if quarantining is not enabled, return exit code and empty quarantine status if quarantine_config @@ -691,6 +690,7 @@ mod tests { repo: RepoUrlParts::default(), org_url_slug: String::new(), fetch_status: QuarantineFetchStatus::FetchSucceeded, + quarantine_resolution_mode: api::message::QuarantineResolutionMode::Unspecified, }; assert_eq!( quarantine_query_result(false, &success_ctx), @@ -720,6 +720,7 @@ mod tests { failures: vec![], repo: RepoUrlParts::default(), org_url_slug: String::new(), + quarantine_resolution_mode: api::message::QuarantineResolutionMode::Unspecified, }; assert_eq!( quarantine_query_result(false, &repo_disabled_ctx), diff --git a/test_report/src/report.rs b/test_report/src/report.rs index 8fe47fb7..804645e5 100644 --- a/test_report/src/report.rs +++ b/test_report/src/report.rs @@ -7,7 +7,10 @@ use std::{ time::{Duration, SystemTime}, }; -use api::{client::ApiClient, message}; +use api::{ + client::ApiClient, + message::{self, QuarantineResolutionMode}, +}; use bundle::BundleMetaDebugProps; use bundle::Test; use chrono::prelude::*; @@ -42,6 +45,8 @@ struct QuarantineConfig { quarantining_disabled_for_repo: bool, quarantine_lookup_failed: bool, quarantined_tests: HashMap, + #[serde(default)] + quarantine_resolution_mode: QuarantineResolutionMode, } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -65,9 +70,6 @@ pub struct TestReport { started_at: SystemTime, quarantine_config: Option, quarantine_lookup_source: Option, - /// Which source the server resolved quarantine status from ("repo" or - /// "test_collection"), captured from the getQuarantineConfig response. - quarantine_resolution_mode: Option, quarantined_tests_disk_cache_ttl: Duration, codeowners: Option, variant: Option, @@ -242,7 +244,6 @@ impl MutTestReport { started_at, quarantine_config: None, quarantine_lookup_source: None, - quarantine_resolution_mode: None, quarantined_tests_disk_cache_ttl, codeowners, repo, @@ -271,13 +272,13 @@ impl MutTestReport { fn quarantine_resolution_mode_for_telemetry( &self, ) -> proto::upload_metrics::trunk::QuarantineResolutionMode { - use proto::upload_metrics::trunk::QuarantineResolutionMode; - - match self.0.borrow().quarantine_resolution_mode.as_deref() { - Some("repo") => QuarantineResolutionMode::Repo, - Some("test_collection") => QuarantineResolutionMode::TestCollection, - _ => QuarantineResolutionMode::Unspecified, - } + self.0 + .borrow() + .quarantine_config + .as_ref() + .map(|config| config.quarantine_resolution_mode) + .unwrap_or_default() + .into() } fn serialize_test_report(&self) -> Vec { @@ -530,6 +531,29 @@ impl MutTestReport { } } + /// Log which source the server resolved quarantine status from. Callers invoke + /// this only on the first (memoized) lookup, so it fires at most once per report. + fn log_quarantine_resolution_mode( + &self, + resolution_mode: QuarantineResolutionMode, + repo: &RepoUrlParts, + ) { + match resolution_mode { + QuarantineResolutionMode::TestCollection => tracing::info!( + "Resolved quarantine status for test collection {}", + env::var(constants::TRUNK_TEST_COLLECTION_ID_ENV) + .ok() + .as_deref() + .unwrap_or("unknown"), + ), + QuarantineResolutionMode::Repo => tracing::info!( + "Resolved quarantine status for repo {}", + repo.repo_full_name(), + ), + QuarantineResolutionMode::Unspecified => {} + } + } + fn populate_quarantined_tests( &self, api_client: &ApiClient, @@ -544,7 +568,9 @@ impl MutTestReport { if let Some(cache_entry) = self.load_quarantine_config_from_disk_cache(&org_url_slug, &repo_url) { - self.0.borrow_mut().quarantine_config = Some(cache_entry.quarantine_config); + let quarantine_config = cache_entry.quarantine_config; + self.log_quarantine_resolution_mode(quarantine_config.quarantine_resolution_mode, repo); + self.0.borrow_mut().quarantine_config = Some(quarantine_config); self.record_quarantine_lookup_source(QuarantineLookupSource::DiskCache); return; } @@ -562,22 +588,15 @@ impl MutTestReport { .build() .unwrap() .block_on(api_client.get_quarantining_config(&request)); + let mut resolution_mode = QuarantineResolutionMode::Unspecified; let (quarantining_disabled, quarantine_lookup_failed) = match response { Ok(response) => { let is_disabled = response.is_disabled; for quarantined_test_id in response.quarantined_tests.iter() { quarantined_tests.insert(quarantined_test_id.clone(), true); } - // Surface which source the server resolved quarantine status from. Sent to Sentry - // only (hidden from the console) to aid debugging without adding CLI noise. - tracing::info!( - hidden_in_console = true, - quarantine_resolution_mode = - response.quarantine_resolution_mode.as_deref().unwrap_or("unspecified"), - "Resolved quarantine config" - ); - self.0.borrow_mut().quarantine_resolution_mode = - response.quarantine_resolution_mode.clone(); + resolution_mode = response.quarantine_resolution_mode; + self.log_quarantine_resolution_mode(resolution_mode, repo); if is_disabled { self.record_quarantine_lookup_source(QuarantineLookupSource::Disabled); } else { @@ -601,6 +620,7 @@ impl MutTestReport { quarantining_disabled_for_repo: quarantining_disabled, quarantine_lookup_failed, quarantined_tests: quarantined_tests.clone(), + quarantine_resolution_mode: resolution_mode, }; self.0.borrow_mut().quarantine_config = Some(quarantine_config.clone()); if !quarantine_lookup_failed { From 084a6ea9e6539f2e2890830fedbacd3fec12bb9b Mon Sep 17 00:00:00 2001 From: max-trunk Date: Fri, 24 Jul 2026 21:12:48 +0000 Subject: [PATCH 3/5] =?UTF-8?q?fix(quarantine):=20address=20review=20?= =?UTF-8?q?=E2=80=94=20thread=20test=20collection=20id,=20tolerant=20parse?= =?UTF-8?q?,=20cache=20key?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - BLOCKING: the traditional CLI path built getQuarantineConfig with test_collection_short_id always None. meta.base_props.test_collection isn't populated until upload_bundle (after the request is built), and only when the upload-intent response returns collection metadata, so the CLI value was silently dropped. Thread upload_args.test_collection_short_id (empty-filtered) into gather_exit_code_and_quarantined_tests_context and use it directly, matching the rspec path. - Include the collection id in the rspec disk-cache key so two runs in the same repo with different TRUNK_TEST_COLLECTION_ID values can't share an entry (and the wrong quarantine list). - Make QuarantineResolutionMode's deserializer tolerant of non-string JSON types (number/bool/object/null), not just unknown strings. - Filter empty TRUNK_TEST_COLLECTION_ID so an empty env var doesn't send "". - Collapse the duplicated resolution-mode log message into a QuarantineResolutionMode::resolution_log_line helper; mark cached lookups. - Inline the trivial quarantine_resolution_mode() mapping fn at its one call site; use the imported enum name consistently. - Tests: assert the collection id reaches the getQuarantineConfig request on the traditional path (cli/tests/upload.rs), plus a non-string deserialize case. Co-Authored-By: Claude Opus 4.8 --- api/src/message.rs | 52 ++++++++++++++++++--- cli/src/context.rs | 7 +-- cli/src/context_quarantine.rs | 33 +++++-------- cli/src/upload_command.rs | 10 ++-- cli/tests/upload.rs | 5 ++ test_report/src/report.rs | 87 +++++++++++++++++++++-------------- 6 files changed, 122 insertions(+), 72 deletions(-) diff --git a/api/src/message.rs b/api/src/message.rs index 2a017f09..c2b0e603 100644 --- a/api/src/message.rs +++ b/api/src/message.rs @@ -42,13 +42,15 @@ impl<'de> Deserialize<'de> for QuarantineResolutionMode { where D: serde::Deserializer<'de>, { - Ok( - match Option::::deserialize(deserializer)?.as_deref() { - Some("repo") => Self::Repo, - Some("test_collection") => Self::TestCollection, - _ => Self::Unspecified, - }, - ) + // Tolerant: any unknown/absent value — and any non-string JSON type + // (number, bool, object, null) — degrades to Unspecified rather than + // failing the whole response parse, so a new server value can't break + // older clients. + Ok(match serde_json::Value::deserialize(deserializer)?.as_str() { + Some("repo") => Self::Repo, + Some("test_collection") => Self::TestCollection, + _ => Self::Unspecified, + }) } } @@ -62,6 +64,29 @@ impl From for proto::upload_metrics::trunk::Quarantine } } +impl QuarantineResolutionMode { + /// Human-readable line describing a resolved quarantine lookup, or `None` when + /// the mode is unspecified (nothing worth logging). `test_collection_id` is + /// used only for `TestCollection`, `repo` only for `Repo`. + pub fn resolution_log_line( + &self, + test_collection_id: Option<&str>, + repo: &RepoUrlParts, + ) -> Option { + match self { + Self::TestCollection => Some(format!( + "Resolved quarantine status for test collection {}", + test_collection_id.unwrap_or("unknown"), + )), + Self::Repo => Some(format!( + "Resolved quarantine status for repo {}", + repo.repo_full_name() + )), + Self::Unspecified => None, + } + } +} + #[derive(Debug, Serialize, Clone, Deserialize, Default)] #[serde(rename_all = "camelCase")] pub struct GetQuarantineConfigResponse { @@ -176,4 +201,17 @@ mod tests { QuarantineResolutionMode::Unspecified ); } + + #[test] + fn deserializes_non_string_quarantine_resolution_mode_as_unspecified() { + // A non-string JSON type must not fail the whole response parse either. + let response: GetQuarantineConfigResponse = serde_json::from_str( + r#"{ "isDisabled": false, "testIds": [], "quarantineResolutionMode": 3 }"#, + ) + .unwrap(); + assert_eq!( + response.quarantine_resolution_mode, + QuarantineResolutionMode::Unspecified + ); + } } diff --git a/cli/src/context.rs b/cli/src/context.rs index 2f8bf97e..6122c914 100644 --- a/cli/src/context.rs +++ b/cli/src/context.rs @@ -770,6 +770,7 @@ pub async fn gather_exit_code_and_quarantined_tests_context( api_client: &ApiClient, file_set_builder: &FileSetBuilder, default_exit_code: Option, + test_collection_short_id: Option, ) -> anyhow::Result { // Run the quarantine step and update the exit code. let failed_tests_extractor = FailedTestsExtractor::new( @@ -815,11 +816,7 @@ pub async fn gather_exit_code_and_quarantined_tests_context( org_url_slug: meta.base_props.org.clone(), test_identifiers: failed_tests_extractor.failed_tests().to_vec(), remote_urls: vec![meta.base_props.repo.repo_url.clone()], - test_collection_short_id: meta - .base_props - .test_collection - .as_ref() - .map(|tc| tc.short_id.clone()), + test_collection_short_id, }, file_set_builder, Some(failed_tests_extractor), diff --git a/cli/src/context_quarantine.rs b/cli/src/context_quarantine.rs index f3bdb52b..9bb400e6 100644 --- a/cli/src/context_quarantine.rs +++ b/cli/src/context_quarantine.rs @@ -47,7 +47,7 @@ pub struct QuarantineContext { pub repo: RepoUrlParts, pub org_url_slug: String, pub fetch_status: QuarantineFetchStatus, - pub quarantine_resolution_mode: api::message::QuarantineResolutionMode, + pub quarantine_resolution_mode: QuarantineResolutionMode, } impl QuarantineContext { pub fn skip_fetch(failures: Vec) -> Self { @@ -58,7 +58,7 @@ impl QuarantineContext { repo: RepoUrlParts::default(), org_url_slug: String::default(), fetch_status: QuarantineFetchStatus::FetchSkipped, - quarantine_resolution_mode: api::message::QuarantineResolutionMode::Unspecified, + quarantine_resolution_mode: QuarantineResolutionMode::Unspecified, } } @@ -70,7 +70,7 @@ impl QuarantineContext { repo: RepoUrlParts::default(), org_url_slug: String::default(), fetch_status: QuarantineFetchStatus::FetchFailed(error), - quarantine_resolution_mode: api::message::QuarantineResolutionMode::Unspecified, + quarantine_resolution_mode: QuarantineResolutionMode::Unspecified, } } } @@ -90,12 +90,6 @@ pub fn quarantine_query_result( } } -pub fn quarantine_resolution_mode( - ctx: &QuarantineContext, -) -> proto::upload_metrics::trunk::QuarantineResolutionMode { - ctx.quarantine_resolution_mode.into() -} - fn convert_case_to_test + ToString>( repo: &RepoUrlParts, org_slug: T, @@ -331,7 +325,7 @@ pub async fn gather_quarantine_context( quarantine_status: QuarantineBulkTestStatus::default(), failures: Vec::default(), fetch_status: QuarantineFetchStatus::FetchSkipped, - quarantine_resolution_mode: api::message::QuarantineResolutionMode::Unspecified, + quarantine_resolution_mode: QuarantineResolutionMode::Unspecified, }); } @@ -347,16 +341,11 @@ pub async fn gather_quarantine_context( tracing::info!("Checking if failed tests can be quarantined"); match api_client.get_quarantining_config(request).await { anyhow::Result::Ok(response) => { - match response.quarantine_resolution_mode { - QuarantineResolutionMode::TestCollection => tracing::info!( - "Resolved quarantine status for test collection {}", - request.test_collection_short_id.as_deref().unwrap_or("unknown"), - ), - QuarantineResolutionMode::Repo => tracing::info!( - "Resolved quarantine status for repo {}", - request.repo.repo_full_name(), - ), - QuarantineResolutionMode::Unspecified => {} + if let Some(line) = response.quarantine_resolution_mode.resolution_log_line( + request.test_collection_short_id.as_deref(), + &request.repo, + ) { + tracing::info!("{line}"); } (Some(response), QuarantineFetchStatus::FetchSucceeded) } @@ -690,7 +679,7 @@ mod tests { repo: RepoUrlParts::default(), org_url_slug: String::new(), fetch_status: QuarantineFetchStatus::FetchSucceeded, - quarantine_resolution_mode: api::message::QuarantineResolutionMode::Unspecified, + quarantine_resolution_mode: QuarantineResolutionMode::Unspecified, }; assert_eq!( quarantine_query_result(false, &success_ctx), @@ -720,7 +709,7 @@ mod tests { failures: vec![], repo: RepoUrlParts::default(), org_url_slug: String::new(), - quarantine_resolution_mode: api::message::QuarantineResolutionMode::Unspecified, + quarantine_resolution_mode: QuarantineResolutionMode::Unspecified, }; assert_eq!( quarantine_query_result(false, &repo_disabled_ctx), diff --git a/cli/src/upload_command.rs b/cli/src/upload_command.rs index e7367256..cf518317 100644 --- a/cli/src/upload_command.rs +++ b/cli/src/upload_command.rs @@ -18,9 +18,7 @@ use superconsole::{ }; use tempfile::TempDir; -use crate::context_quarantine::{ - QuarantineContext, quarantine_query_result, quarantine_resolution_mode, -}; +use crate::context_quarantine::{QuarantineContext, quarantine_query_result}; use crate::validate_command::JunitReportValidations; use crate::{ context::{ @@ -489,6 +487,10 @@ pub async fn run_upload( &api_client, &file_set_builder, default_exit_code, + upload_args + .test_collection_short_id + .clone() + .filter(|id| !id.is_empty()), ) .await { @@ -569,7 +571,7 @@ pub async fn run_upload( let quarantine_query_result = quarantine_query_result_override .unwrap_or_else(|| quarantine_query_result(disable_quarantining, &quarantine_context)); let quarantine_resolution_mode = quarantine_resolution_mode_override - .unwrap_or_else(|| quarantine_resolution_mode(&quarantine_context)); + .unwrap_or_else(|| quarantine_context.quarantine_resolution_mode.into()); let upload_metrics = proto::upload_metrics::trunk::UploadMetrics { client_version: Some(proto::upload_metrics::trunk::Semver { major: env!("CARGO_PKG_VERSION_MAJOR").parse().unwrap_or_default(), diff --git a/cli/tests/upload.rs b/cli/tests/upload.rs index 53e67e45..02770864 100644 --- a/cli/tests/upload.rs +++ b/cli/tests/upload.rs @@ -103,6 +103,11 @@ async fn upload_bundle() { assert_eq!(req.repo.owner, "trunk-io"); assert_eq!(req.repo.name, "analytics-cli"); assert_eq!(req.org_url_slug, "test-org"); + assert_eq!( + req.test_collection_short_id, + Some(String::from("tc_123")), + "--test-collection-id must reach the getQuarantineConfig request" + ); assert!( !req.test_identifiers.is_empty(), "test_identifiers should not be empty" diff --git a/test_report/src/report.rs b/test_report/src/report.rs index 804645e5..4c1dc078 100644 --- a/test_report/src/report.rs +++ b/test_report/src/report.rs @@ -438,10 +438,22 @@ impl MutTestReport { } } - fn get_quarantine_config_cache_file_path(&self, org_url_slug: &str, repo_url: &str) -> PathBuf { + fn get_quarantine_config_cache_file_path( + &self, + org_url_slug: &str, + repo_url: &str, + test_collection_short_id: Option<&str>, + ) -> PathBuf { + // The collection id is part of the key: quarantine status now resolves + // per test collection, so two runs in the same repo with different ids + // must not share a cache entry. let cache_key = Uuid::new_v5( &Uuid::NAMESPACE_URL, - format!("{org_url_slug}#{repo_url}").as_bytes(), + format!( + "{org_url_slug}#{repo_url}#{}", + test_collection_short_id.unwrap_or_default() + ) + .as_bytes(), ) .to_string(); let quarantine_config_cache_file_name = format!("quarantine_config_{cache_key}.json"); @@ -453,8 +465,13 @@ impl MutTestReport { &self, org_url_slug: &str, repo_url: &str, + test_collection_short_id: Option<&str>, ) -> Option { - let cache_path = self.get_quarantine_config_cache_file_path(org_url_slug, repo_url); + let cache_path = self.get_quarantine_config_cache_file_path( + org_url_slug, + repo_url, + test_collection_short_id, + ); let cache_file = match fs::File::open(&cache_path) { Ok(file) => file, @@ -494,9 +511,14 @@ impl MutTestReport { &self, org_url_slug: &str, repo_url: &str, + test_collection_short_id: Option<&str>, quarantine_config: &QuarantineConfig, ) { - let cache_path = self.get_quarantine_config_cache_file_path(org_url_slug, repo_url); + let cache_path = self.get_quarantine_config_cache_file_path( + org_url_slug, + repo_url, + test_collection_short_id, + ); let now = match SystemTime::now().duration_since(SystemTime::UNIX_EPOCH) { Ok(duration) => duration.as_secs(), @@ -531,29 +553,6 @@ impl MutTestReport { } } - /// Log which source the server resolved quarantine status from. Callers invoke - /// this only on the first (memoized) lookup, so it fires at most once per report. - fn log_quarantine_resolution_mode( - &self, - resolution_mode: QuarantineResolutionMode, - repo: &RepoUrlParts, - ) { - match resolution_mode { - QuarantineResolutionMode::TestCollection => tracing::info!( - "Resolved quarantine status for test collection {}", - env::var(constants::TRUNK_TEST_COLLECTION_ID_ENV) - .ok() - .as_deref() - .unwrap_or("unknown"), - ), - QuarantineResolutionMode::Repo => tracing::info!( - "Resolved quarantine status for repo {}", - repo.repo_full_name(), - ), - QuarantineResolutionMode::Unspecified => {} - } - } - fn populate_quarantined_tests( &self, api_client: &ApiClient, @@ -565,11 +564,22 @@ impl MutTestReport { return; } - if let Some(cache_entry) = - self.load_quarantine_config_from_disk_cache(&org_url_slug, &repo_url) - { + let test_collection_short_id = env::var(constants::TRUNK_TEST_COLLECTION_ID_ENV) + .ok() + .filter(|id| !id.is_empty()); + + if let Some(cache_entry) = self.load_quarantine_config_from_disk_cache( + &org_url_slug, + &repo_url, + test_collection_short_id.as_deref(), + ) { let quarantine_config = cache_entry.quarantine_config; - self.log_quarantine_resolution_mode(quarantine_config.quarantine_resolution_mode, repo); + if let Some(line) = quarantine_config + .quarantine_resolution_mode + .resolution_log_line(test_collection_short_id.as_deref(), repo) + { + tracing::info!("{line} (cached)"); + } self.0.borrow_mut().quarantine_config = Some(quarantine_config); self.record_quarantine_lookup_source(QuarantineLookupSource::DiskCache); return; @@ -581,7 +591,7 @@ impl MutTestReport { test_identifiers: vec![], remote_urls: vec![repo_url.clone()], repo: repo.clone(), - test_collection_short_id: env::var(constants::TRUNK_TEST_COLLECTION_ID_ENV).ok(), + test_collection_short_id: test_collection_short_id.clone(), }; let response = tokio::runtime::Builder::new_multi_thread() .enable_all() @@ -596,7 +606,11 @@ impl MutTestReport { quarantined_tests.insert(quarantined_test_id.clone(), true); } resolution_mode = response.quarantine_resolution_mode; - self.log_quarantine_resolution_mode(resolution_mode, repo); + if let Some(line) = + resolution_mode.resolution_log_line(test_collection_short_id.as_deref(), repo) + { + tracing::info!("{line}"); + } if is_disabled { self.record_quarantine_lookup_source(QuarantineLookupSource::Disabled); } else { @@ -624,7 +638,12 @@ impl MutTestReport { }; self.0.borrow_mut().quarantine_config = Some(quarantine_config.clone()); if !quarantine_lookup_failed { - self.save_quarantine_config_to_disk_cache(&org_url_slug, &repo_url, &quarantine_config); + self.save_quarantine_config_to_disk_cache( + &org_url_slug, + &repo_url, + test_collection_short_id.as_deref(), + &quarantine_config, + ); } } From 28ccff8b33e3219d44582d26f7d19741cb114939 Mon Sep 17 00:00:00 2001 From: max-trunk Date: Mon, 27 Jul 2026 14:17:55 +0000 Subject: [PATCH 4/5] chore(quarantine): trim comments and drop low-value message.rs unit tests - Shorten the QuarantineResolutionMode doc comment; drop the deserializer and resolution_log_line comments in api/src/message.rs. - Remove the api/src/message.rs serde unit tests (type-only, low value). - Prune the per-variant and trailing comments on the proto QuarantineResolutionMode enum, keeping only the top-level line. Co-Authored-By: Claude Opus 4.8 --- api/src/message.rs | 97 +------------------------------- proto/proto/upload_metrics.proto | 6 +- 2 files changed, 2 insertions(+), 101 deletions(-) diff --git a/api/src/message.rs b/api/src/message.rs index c2b0e603..1c3c2095 100644 --- a/api/src/message.rs +++ b/api/src/message.rs @@ -24,10 +24,7 @@ pub struct CreateBundleUploadResponse { pub test_collection_bundle_meta_created_at: Option, } -/// Which source the server resolved quarantine status from, mapped from the -/// getQuarantineConfig response's `quarantineResolutionMode` string. Unknown or -/// absent values deserialize to `Unspecified` so a new server-side value never -/// breaks deserialization on older clients. +/// Which source the server resolved quarantine status from. #[derive(Debug, Serialize, Clone, Copy, PartialEq, Eq, Default)] #[serde(rename_all = "snake_case")] pub enum QuarantineResolutionMode { @@ -42,10 +39,6 @@ impl<'de> Deserialize<'de> for QuarantineResolutionMode { where D: serde::Deserializer<'de>, { - // Tolerant: any unknown/absent value — and any non-string JSON type - // (number, bool, object, null) — degrades to Unspecified rather than - // failing the whole response parse, so a new server value can't break - // older clients. Ok(match serde_json::Value::deserialize(deserializer)?.as_str() { Some("repo") => Self::Repo, Some("test_collection") => Self::TestCollection, @@ -65,9 +58,6 @@ impl From for proto::upload_metrics::trunk::Quarantine } impl QuarantineResolutionMode { - /// Human-readable line describing a resolved quarantine lookup, or `None` when - /// the mode is unspecified (nothing worth logging). `test_collection_id` is - /// used only for `TestCollection`, `repo` only for `Repo`. pub fn resolution_log_line( &self, test_collection_id: Option<&str>, @@ -130,88 +120,3 @@ pub struct CreateBundleUploadIntentResponse { pub struct TelemetryUploadMetricsRequest { pub upload_metrics: proto::upload_metrics::trunk::UploadMetrics, } - -#[cfg(test)] -mod tests { - use super::*; - - fn base_request() -> GetQuarantineConfigRequest { - GetQuarantineConfigRequest { - repo: RepoUrlParts { - host: String::from("github.com"), - owner: String::from("trunk-io"), - name: String::from("analytics-cli"), - }, - remote_urls: vec![], - org_url_slug: String::from("trunk"), - test_identifiers: vec![], - test_collection_short_id: None, - } - } - - #[test] - fn serializes_test_collection_short_id_as_camel_case_when_set() { - let request = GetQuarantineConfigRequest { - test_collection_short_id: Some(String::from("abcd1234")), - ..base_request() - }; - let value: serde_json::Value = serde_json::to_value(&request).unwrap(); - assert_eq!(value["testCollectionShortId"], "abcd1234"); - } - - #[test] - fn omits_test_collection_short_id_when_absent() { - let value: serde_json::Value = serde_json::to_value(base_request()).unwrap(); - assert!(value.get("testCollectionShortId").is_none()); - } - - #[test] - fn deserializes_response_with_quarantine_resolution_mode() { - let response: GetQuarantineConfigResponse = serde_json::from_str( - r#"{ "isDisabled": false, "testIds": ["id1"], "quarantineResolutionMode": "test_collection" }"#, - ) - .unwrap(); - assert_eq!( - response.quarantine_resolution_mode, - QuarantineResolutionMode::TestCollection - ); - } - - #[test] - fn deserializes_response_without_quarantine_resolution_mode() { - // Older servers omit the field; deserialization must still succeed. - let response: GetQuarantineConfigResponse = - serde_json::from_str(r#"{ "isDisabled": false, "testIds": [] }"#).unwrap(); - assert_eq!( - response.quarantine_resolution_mode, - QuarantineResolutionMode::Unspecified - ); - } - - #[test] - fn deserializes_unknown_quarantine_resolution_mode_as_unspecified() { - // A resolution mode the client doesn't recognize must not fail the whole - // response parse; it degrades to Unspecified. - let response: GetQuarantineConfigResponse = serde_json::from_str( - r#"{ "isDisabled": false, "testIds": [], "quarantineResolutionMode": "something_new" }"#, - ) - .unwrap(); - assert_eq!( - response.quarantine_resolution_mode, - QuarantineResolutionMode::Unspecified - ); - } - - #[test] - fn deserializes_non_string_quarantine_resolution_mode_as_unspecified() { - // A non-string JSON type must not fail the whole response parse either. - let response: GetQuarantineConfigResponse = serde_json::from_str( - r#"{ "isDisabled": false, "testIds": [], "quarantineResolutionMode": 3 }"#, - ) - .unwrap(); - assert_eq!( - response.quarantine_resolution_mode, - QuarantineResolutionMode::Unspecified - ); - } -} diff --git a/proto/proto/upload_metrics.proto b/proto/proto/upload_metrics.proto index 47d459ec..2a14c8c9 100644 --- a/proto/proto/upload_metrics.proto +++ b/proto/proto/upload_metrics.proto @@ -20,14 +20,10 @@ enum QuarantineQueryResult { QUARANTINE_QUERY_RESULT_CACHED = 5; } -// Which source the server used to resolve quarantine status, from the -// getQuarantineConfig response's `quarantineResolutionMode` field. +// Which source the server used to resolve quarantine status enum QuarantineResolutionMode { - // Default; the response omitted the field (older server) or no lookup ran. QUARANTINE_RESOLUTION_MODE_UNSPECIFIED = 0; - // Quarantine status was resolved from the repo. QUARANTINE_RESOLUTION_MODE_REPO = 1; - // Quarantine status was resolved from the test collection. QUARANTINE_RESOLUTION_MODE_TEST_COLLECTION = 2; } From 509680e17082378fafbb49eb5518cabeb0fd4065 Mon Sep 17 00:00:00 2001 From: max-trunk Date: Mon, 27 Jul 2026 14:21:37 +0000 Subject: [PATCH 5/5] chore(quarantine): drop test_collection_short_id field comment Co-Authored-By: Claude Opus 4.8 --- api/src/message.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/api/src/message.rs b/api/src/message.rs index 1c3c2095..74d662bf 100644 --- a/api/src/message.rs +++ b/api/src/message.rs @@ -94,8 +94,6 @@ pub struct GetQuarantineConfigRequest { pub remote_urls: Vec, pub org_url_slug: String, pub test_identifiers: Vec, - /// Optional test collection short id (from `--test-collection-id`). When set and the org is - /// migrated to test collections, the server resolves quarantine status from the collection. #[serde(default, skip_serializing_if = "Option::is_none")] pub test_collection_short_id: Option, }