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..74d662bf 100644 --- a/api/src/message.rs +++ b/api/src/message.rs @@ -24,12 +24,67 @@ pub struct CreateBundleUploadResponse { pub test_collection_bundle_meta_created_at: Option, } +/// Which source the server resolved quarantine status from. +#[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 serde_json::Value::deserialize(deserializer)?.as_str() { + 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, + } + } +} + +impl QuarantineResolutionMode { + 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 { pub is_disabled: bool, #[serde(rename = "testIds")] pub quarantined_tests: Vec, + #[serde(default)] + pub quarantine_resolution_mode: QuarantineResolutionMode, } #[derive(Debug, Serialize, Clone, Deserialize, PartialEq, Eq)] @@ -39,6 +94,8 @@ pub struct GetQuarantineConfigRequest { pub remote_urls: Vec, pub org_url_slug: String, pub test_identifiers: Vec, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub test_collection_short_id: Option, } #[derive(Debug, Serialize, Clone, Deserialize, PartialEq, Eq)] diff --git a/cli/src/context.rs b/cli/src/context.rs index f4a9ce42..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( @@ -801,6 +802,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: api::message::QuarantineResolutionMode::Unspecified, } } else { // default to success if no test run result (i.e. `upload`) @@ -814,6 +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, }, file_set_builder, Some(failed_tests_extractor), diff --git a/cli/src/context_quarantine.rs b/cli/src/context_quarantine.rs index 7146f4ce..9bb400e6 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, }; @@ -47,6 +47,7 @@ pub struct QuarantineContext { pub repo: RepoUrlParts, pub org_url_slug: String, pub fetch_status: QuarantineFetchStatus, + pub quarantine_resolution_mode: QuarantineResolutionMode, } impl QuarantineContext { pub fn skip_fetch(failures: Vec) -> Self { @@ -57,6 +58,7 @@ impl QuarantineContext { repo: RepoUrlParts::default(), org_url_slug: String::default(), fetch_status: QuarantineFetchStatus::FetchSkipped, + quarantine_resolution_mode: QuarantineResolutionMode::Unspecified, } } @@ -68,6 +70,7 @@ impl QuarantineContext { repo: RepoUrlParts::default(), org_url_slug: String::default(), fetch_status: QuarantineFetchStatus::FetchFailed(error), + quarantine_resolution_mode: QuarantineResolutionMode::Unspecified, } } } @@ -322,6 +325,7 @@ pub async fn gather_quarantine_context( quarantine_status: QuarantineBulkTestStatus::default(), failures: Vec::default(), fetch_status: QuarantineFetchStatus::FetchSkipped, + quarantine_resolution_mode: QuarantineResolutionMode::Unspecified, }); } @@ -336,7 +340,15 @@ 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) => { + 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) + } anyhow::Result::Err(error) => (None, QuarantineFetchStatus::FetchFailed(error)), } } else { @@ -344,6 +356,11 @@ pub async fn gather_quarantine_context( (None, QuarantineFetchStatus::FetchSkipped) }; + let quarantine_resolution_mode = quarantine_config + .as_ref() + .map(|config| config.quarantine_resolution_mode) + .unwrap_or_default(); + // if quarantining is not enabled, return exit code and empty quarantine status if quarantine_config .as_ref() @@ -361,6 +378,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 +465,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, }) } @@ -660,6 +679,7 @@ mod tests { repo: RepoUrlParts::default(), org_url_slug: String::new(), fetch_status: QuarantineFetchStatus::FetchSucceeded, + quarantine_resolution_mode: QuarantineResolutionMode::Unspecified, }; assert_eq!( quarantine_query_result(false, &success_ctx), @@ -689,6 +709,7 @@ mod tests { failures: vec![], repo: RepoUrlParts::default(), org_url_slug: String::new(), + 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 e95f5707..cf518317 100644 --- a/cli/src/upload_command.rs +++ b/cli/src/upload_command.rs @@ -380,6 +380,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 +391,7 @@ impl Default for RunUploadOptions { test_run_result: None, render_sender: None, quarantine_query_result_override: None, + quarantine_resolution_mode_override: None, } } } @@ -402,6 +405,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() { @@ -483,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 { @@ -562,6 +570,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_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(), @@ -580,6 +590,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..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" @@ -1294,6 +1299,7 @@ async fn quarantines_tests_regardless_of_upload() { Json(GetQuarantineConfigResponse { is_disabled, quarantined_tests, + ..Default::default() }) } }, @@ -2041,6 +2047,7 @@ async fn do_not_quarantines_tests_when_quarantine_disabled_set() { Json(GetQuarantineConfigResponse { is_disabled, quarantined_tests, + ..Default::default() }) } }, @@ -2149,6 +2156,7 @@ async fn still_quarantines_if_upload_to_s3_fails() { Json(GetQuarantineConfigResponse { is_disabled: false, quarantined_tests: test_ids, + ..Default::default() }) } }, @@ -2189,6 +2197,7 @@ async fn still_quarantines_if_upload_endpoint_fails() { Json(GetQuarantineConfigResponse { is_disabled: false, quarantined_tests: test_ids, + ..Default::default() }) } }, @@ -2229,6 +2238,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 +2280,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..2a14c8c9 100644 --- a/proto/proto/upload_metrics.proto +++ b/proto/proto/upload_metrics.proto @@ -20,6 +20,13 @@ enum QuarantineQueryResult { QUARANTINE_QUERY_RESULT_CACHED = 5; } +// Which source the server used to resolve quarantine status +enum QuarantineResolutionMode { + QUARANTINE_RESOLUTION_MODE_UNSPECIFIED = 0; + QUARANTINE_RESOLUTION_MODE_REPO = 1; + QUARANTINE_RESOLUTION_MODE_TEST_COLLECTION = 2; +} + message UploadMetrics { Semver client_version = 1; Repo repo = 2; @@ -29,6 +36,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..4c1dc078 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)] @@ -264,6 +269,18 @@ impl MutTestReport { } } + fn quarantine_resolution_mode_for_telemetry( + &self, + ) -> proto::upload_metrics::trunk::QuarantineResolutionMode { + self.0 + .borrow() + .quarantine_config + .as_ref() + .map(|config| config.quarantine_resolution_mode) + .unwrap_or_default() + .into() + } + fn serialize_test_report(&self) -> Vec { prost::Message::encode_to_vec(&self.0.borrow().test_report) } @@ -421,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"); @@ -436,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, @@ -477,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(), @@ -525,10 +564,23 @@ impl MutTestReport { return; } - 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 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; + 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; } @@ -539,18 +591,26 @@ impl MutTestReport { test_identifiers: vec![], remote_urls: vec![repo_url.clone()], repo: repo.clone(), + test_collection_short_id: test_collection_short_id.clone(), }; let response = tokio::runtime::Builder::new_multi_thread() .enable_all() .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); } + resolution_mode = response.quarantine_resolution_mode; + 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 { @@ -574,10 +634,16 @@ 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 { - 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, + ); } } @@ -724,6 +790,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() }) }