diff --git a/api/src/message.rs b/api/src/message.rs index 74d662bf..9539908b 100644 --- a/api/src/message.rs +++ b/api/src/message.rs @@ -1,3 +1,4 @@ +pub use bundle::QuarantineResolutionMode; use bundle::Test; use context::repo::RepoUrlParts; use serde::{Deserialize, Serialize}; @@ -24,59 +25,6 @@ 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 { diff --git a/bundle/src/bundle_meta.rs b/bundle/src/bundle_meta.rs index e6b4ec48..5abedb67 100644 --- a/bundle/src/bundle_meta.rs +++ b/bundle/src/bundle_meta.rs @@ -20,7 +20,7 @@ use tsify_next::Tsify; use wasm_bindgen::prelude::*; use crate::{ - CustomTag, Test, + CustomTag, QuarantineResolutionMode, Test, files::{BundledFile, FileSet}, }; @@ -58,6 +58,8 @@ pub struct BundleMetaBaseProps { pub quarantined_tests: Vec, pub codeowners: Option, pub use_uncloned_repo: Option, + #[serde(default)] + pub quarantine_resolution_mode: QuarantineResolutionMode, } #[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)] #[cfg_attr(feature = "pyo3", gen_stub_pyclass, pyclass(get_all))] diff --git a/bundle/src/bundler.rs b/bundle/src/bundler.rs index 0976c852..ad153a79 100644 --- a/bundle/src/bundler.rs +++ b/bundle/src/bundler.rs @@ -337,7 +337,7 @@ mod tests { use super::{archive_entries, parse_internal_bin_entry}; use crate::{ - BundledFile, Test, VersionedBundle, + BundledFile, QuarantineResolutionMode, Test, VersionedBundle, bundle_meta::{ BundleMeta, BundleMetaBaseProps, BundleMetaDebugProps, BundleMetaJunitProps, META_VERSION, @@ -428,6 +428,7 @@ mod tests { codeowners: None, envs, use_uncloned_repo: None, + quarantine_resolution_mode: QuarantineResolutionMode::TestCollection, }, internal_bundled_file, failed_tests: vec![Test::new( diff --git a/bundle/src/lib.rs b/bundle/src/lib.rs index b8f439b0..d98bdd50 100644 --- a/bundle/src/lib.rs +++ b/bundle/src/lib.rs @@ -1,9 +1,11 @@ mod bundle_meta; mod bundler; mod files; +mod quarantine_resolution_mode; mod types; pub use bundle_meta::*; pub use bundler::*; pub use files::*; +pub use quarantine_resolution_mode::*; pub use types::*; diff --git a/bundle/src/quarantine_resolution_mode.rs b/bundle/src/quarantine_resolution_mode.rs new file mode 100644 index 00000000..393ef587 --- /dev/null +++ b/bundle/src/quarantine_resolution_mode.rs @@ -0,0 +1,65 @@ +use context::repo::RepoUrlParts; +#[cfg(feature = "pyo3")] +use pyo3::prelude::*; +#[cfg(feature = "pyo3")] +use pyo3_stub_gen::derive::gen_stub_pyclass_enum; +use serde::{Deserialize, Serialize}; +#[cfg(feature = "wasm")] +use tsify_next::Tsify; + +/// Which source the server resolved quarantine status from. +#[derive(Debug, Serialize, Clone, Copy, PartialEq, Eq, Default)] +#[cfg_attr(feature = "pyo3", gen_stub_pyclass_enum, pyclass(eq, eq_int))] +#[cfg_attr(feature = "wasm", derive(Tsify))] +#[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!( + "Quarantine status applied from test collection {}", + test_collection_id.unwrap_or("unknown"), + )), + Self::Repo => Some(format!( + "Quarantine status applied from repo {}", + repo.repo_full_name() + )), + Self::Unspecified => None, + } + } +} diff --git a/bundle/tests/test_collection_props_test.rs b/bundle/tests/test_collection_props_test.rs index 2720426f..4bedd1a1 100644 --- a/bundle/tests/test_collection_props_test.rs +++ b/bundle/tests/test_collection_props_test.rs @@ -1,6 +1,6 @@ use std::collections::HashMap; -use bundle::{BundleMetaBaseProps, TestCollectionProps}; +use bundle::{BundleMetaBaseProps, QuarantineResolutionMode, TestCollectionProps}; use context::repo::BundleRepo; use serde_json::json; @@ -21,6 +21,7 @@ fn build_base_props(test_collection: Option) -> BundleMetaB quarantined_tests: vec![], codeowners: None, use_uncloned_repo: None, + quarantine_resolution_mode: QuarantineResolutionMode::Unspecified, } } diff --git a/cli/src/context.rs b/cli/src/context.rs index 6122c914..1e5da157 100644 --- a/cli/src/context.rs +++ b/cli/src/context.rs @@ -13,7 +13,7 @@ use api::{client::ApiClient, message::CreateBundleUploadResponse}; use bundle::{ BundleMeta, BundleMetaBaseProps, BundleMetaDebugProps, BundleMetaJunitProps, BundledFile, FileSet, FileSetBuilder, FileSetType, INTERNAL_BIN_FILENAME, META_VERSION, - QuarantineBulkTestStatus, TestCollectionProps, bin_parse, + QuarantineBulkTestStatus, QuarantineResolutionMode, TestCollectionProps, bin_parse, }; use codeowners::{CodeOwners, OwnersSource}; use constants::{ENVS_TO_GET, TRUNK_API_TOKEN_ENV, TRUNK_ENVS_TO_CAPTURE, TRUNK_PR_NUMBER_ENV}; @@ -194,6 +194,7 @@ pub fn gather_initial_test_context( os_info: Some(env::consts::OS.to_string()), codeowners: None, use_uncloned_repo: Some(upload_args.use_uncloned_repo), + quarantine_resolution_mode: QuarantineResolutionMode::Unspecified, }, failed_tests: Vec::with_capacity(0), variant: upload_args.variant.as_ref().map(|v| { diff --git a/cli/src/upload_command.rs b/cli/src/upload_command.rs index cf518317..426dca44 100644 --- a/cli/src/upload_command.rs +++ b/cli/src/upload_command.rs @@ -5,7 +5,7 @@ use std::sync::mpsc::Sender; use api::client::{ApiClient, ApiErrorEndpoint}; use api::{client::get_api_host, urls::url_for_test_case}; -use bundle::{BundleMeta, BundlerUtil, Test, unzip_tarball}; +use bundle::{BundleMeta, BundlerUtil, QuarantineResolutionMode, Test, unzip_tarball}; use clap::{ArgAction, Args}; use codeowners::OwnersSource; use constants::EXIT_SUCCESS; @@ -380,8 +380,7 @@ pub struct RunUploadOptions { pub render_sender: Option>, pub quarantine_query_result_override: Option, - pub quarantine_resolution_mode_override: - Option, + pub quarantine_resolution_mode_override: Option, } impl Default for RunUploadOptions { @@ -555,6 +554,10 @@ pub async fn run_upload( .quarantine_results .clone(); + let quarantine_resolution_mode = quarantine_resolution_mode_override + .unwrap_or(quarantine_context.quarantine_resolution_mode); + meta.base_props.quarantine_resolution_mode = quarantine_resolution_mode; + meta.failed_tests = quarantine_context.failures.clone(); let upload_started_at = chrono::Utc::now(); @@ -570,8 +573,6 @@ 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(), @@ -590,7 +591,10 @@ pub async fn run_upload( failed: false, failure_reason: "".into(), quarantine_query_result: quarantine_query_result.into(), - quarantine_resolution_mode: quarantine_resolution_mode.into(), + quarantine_resolution_mode: proto::upload_metrics::trunk::QuarantineResolutionMode::from( + quarantine_resolution_mode, + ) + .into(), }; let mut request = api::message::TelemetryUploadMetricsRequest { upload_metrics }; if !upload_args.dry_run { diff --git a/cli/tests/upload.rs b/cli/tests/upload.rs index 02770864..5ef44723 100644 --- a/cli/tests/upload.rs +++ b/cli/tests/upload.rs @@ -11,7 +11,9 @@ use assert_matches::assert_matches; use axum::body::{Body, Bytes}; use axum::response::IntoResponse; use axum::{Json, extract::State, http::StatusCode, response::Response}; -use bundle::{BundleMeta, FileSetType, INTERNAL_BIN_FILENAME, TestCollectionProps}; +use bundle::{ + BundleMeta, FileSetType, INTERNAL_BIN_FILENAME, QuarantineResolutionMode, TestCollectionProps, +}; use chrono::{DateTime, TimeDelta}; use clap::Parser; mod common; @@ -211,6 +213,11 @@ async fn upload_bundle() { assert_eq!(base_props.test_command, None); assert!(base_props.os_info.is_some()); assert!(base_props.quarantined_tests.is_empty()); + assert_eq!( + base_props.quarantine_resolution_mode, + QuarantineResolutionMode::Unspecified, + "a server reporting no resolution mode must land as unspecified" + ); assert_eq!(bundle_meta.failed_tests.len(), failure_count); assert_eq!(bundle_meta.bundle_upload_id_v2, "test-bundle-upload-id-v2"); assert_eq!( @@ -292,6 +299,64 @@ async fn upload_bundle() { )); } +// NOTE: must be multi threaded to start a mock server +#[tokio::test(flavor = "multi_thread")] +async fn upload_bundle_records_quarantine_resolution_mode() { + let temp_dir = tempdir().unwrap(); + generate_mock_git_repo(&temp_dir); + generate_mock_valid_junit_xmls(&temp_dir); + + let mut mock_server_builder = MockServerBuilder::new(); + mock_server_builder.set_get_quarantining_config_handler( + |Json(_): Json| async move { + Json(GetQuarantineConfigResponse { + is_disabled: false, + quarantined_tests: Vec::new(), + quarantine_resolution_mode: QuarantineResolutionMode::TestCollection, + }) + }, + ); + let state = mock_server_builder.spawn_mock_server().await; + + CommandBuilder::upload(temp_dir.path(), state.host.clone()) + .command() + .arg("--test-collection-id") + .arg("tc_123") + .assert() + // the mock JUnit reports contain unquarantined failures, so the upload itself succeeds + // while the command exits non-zero + .failure(); + + let requests = state.requests.lock().unwrap().clone(); + let tar_extract_directory = requests + .iter() + .find_map(|request| match request { + RequestPayload::S3Upload(directory) => Some(directory), + _ => None, + }) + .expect("the bundle must have been uploaded"); + + let file = fs::File::open(tar_extract_directory.join("meta.json")).unwrap(); + let bundle_meta: BundleMeta = serde_json::from_reader(BufReader::new(file)).unwrap(); + assert_eq!( + bundle_meta.base_props.quarantine_resolution_mode, + QuarantineResolutionMode::TestCollection, + "the resolution mode must be persisted in meta.json" + ); + + let upload_metrics = requests + .iter() + .find_map(|request| match request { + RequestPayload::TelemetryUploadMetrics(metrics) => Some(metrics), + _ => None, + }) + .expect("telemetry must have been sent"); + assert_eq!( + upload_metrics.quarantine_resolution_mode, + i32::from(proto::upload_metrics::trunk::QuarantineResolutionMode::TestCollection) + ); +} + // NOTE: must be multi threaded to start a mock server #[tokio::test(flavor = "multi_thread")] async fn upload_bundle_with_public_repo_id() { diff --git a/context-js/tests/parse_compressed_bundle.test.ts b/context-js/tests/parse_compressed_bundle.test.ts index 9438f00f..300833fc 100644 --- a/context-js/tests/parse_compressed_bundle.test.ts +++ b/context-js/tests/parse_compressed_bundle.test.ts @@ -109,6 +109,7 @@ const generateBundleMeta = () => org: faker.company.name(), os_info: process.platform, quarantined_tests: [], + quarantine_resolution_mode: "unspecified", codeowners: { path: faker.system.filePath(), }, diff --git a/context-py/tests/test_parse_meta.py b/context-py/tests/test_parse_meta.py index f68997a5..3c257c14 100644 --- a/context-py/tests/test_parse_meta.py +++ b/context-py/tests/test_parse_meta.py @@ -305,6 +305,8 @@ def test_parse_and_dump_meta_roundtrip(): valid_meta_str = json.dumps(valid_meta, sort_keys=True) expected_meta = dict(valid_meta) expected_meta.pop("test_collection_id") + # Absent from the input, so it round-trips as the default rather than being dropped. + expected_meta["quarantine_resolution_mode"] = "unspecified" expected_meta_str = json.dumps(expected_meta, sort_keys=True) bundle_meta = parse_meta(valid_meta_str.encode()) assert ( diff --git a/test_report/src/report.rs b/test_report/src/report.rs index 4c1dc078..bea22ce8 100644 --- a/test_report/src/report.rs +++ b/test_report/src/report.rs @@ -269,16 +269,13 @@ impl MutTestReport { } } - fn quarantine_resolution_mode_for_telemetry( - &self, - ) -> proto::upload_metrics::trunk::QuarantineResolutionMode { + fn quarantine_resolution_mode(&self) -> QuarantineResolutionMode { self.0 .borrow() .quarantine_config .as_ref() .map(|config| config.quarantine_resolution_mode) .unwrap_or_default() - .into() } fn serialize_test_report(&self) -> Vec { @@ -791,7 +788,7 @@ impl MutTestReport { self.quarantine_query_result_for_telemetry(), ), quarantine_resolution_mode_override: Some( - self.quarantine_resolution_mode_for_telemetry(), + self.quarantine_resolution_mode(), ), ..Default::default() },