From b9e928f1c1f20a089f7cb56f6f3ef6232e89189c Mon Sep 17 00:00:00 2001 From: Tyler Beebe Date: Tue, 3 Nov 2026 21:20:43 +0000 Subject: [PATCH 1/4] feat(cli): use the test collection Trunk infers from the repository A run without --test-collection-id now asks the decision route too, by its repository. When trunk1 answers v2 with a testCollectionShortId, the run uses that collection for the v2 quarantine lookup and upload, as if it had been passed, and warns that 1.0.0 requires the id. A v2 answer with no collection falls back to v1. A run that names its collection ignores any inferred one. The test-runner plugins keep the inferred collection in their API version cache, so every worker process uses it, and print the warning on stderr. Deprecation notices now show in the upload's text output, since the console shows tracing only when verbose. Part of TRUNK-23145 Co-Authored-By: Claude Opus 5.5 --- api/src/client/mod.rs | 50 ++++++--- api/src/message.rs | 44 ++++++++ api/tests/v2_client.rs | 99 ++++++++++++++--- cli/src/summary_output/notice.rs | 2 +- cli/src/summary_output/text.rs | 10 ++ cli/src/upload_command.rs | 38 +++++-- cli/tests/summary/upload.rs | 25 +++++ cli/tests/upload.rs | 180 ++++++++++++++++++++++++------- test_report/src/report.rs | 99 ++++++++++++----- test_report/tests/quarantine.rs | 129 ++++++++++++++++++++++ 10 files changed, 574 insertions(+), 102 deletions(-) diff --git a/api/src/client/mod.rs b/api/src/client/mod.rs index 1814c2ff..2acc5028 100644 --- a/api/src/client/mod.rs +++ b/api/src/client/mod.rs @@ -236,29 +236,53 @@ impl ApiClient { /// Asks Trunk whether this run uses v2. Anything but an explicit `v2`, a failed call /// included, is v1, so this never fails a run. A timeout, a connection error or a 5xx is /// retried once; any other answer is final. + /// + /// A run that names no collection asks too, by its repository: Trunk may answer `v2` with the + /// collection it inferred. A `v2` that names none is v1, since every v2 route needs one. pub async fn get_upload_api_version( &self, request: &message::GetUploadApiVersionRequest, - ) -> UploadApiVersion { - let upload_api_version = if self.trunk_api_v2_client.is_none() - || request - .test_collection_short_id - .as_deref() - .is_none_or(str::is_empty) - { - UploadApiVersion::V1 + ) -> message::UploadApiDecision { + let requested = request + .test_collection_short_id + .as_deref() + .filter(|id| !id.trim().is_empty()); + let has_repo = request + .repo + .as_ref() + .and_then(message::named_repo) + .is_some(); + let decision = if self.trunk_api_v2_client.is_none() || (requested.is_none() && !has_repo) { + message::UploadApiDecision::default() } else { match self.request_upload_api_version(request).await { - Ok(response) if response.api_version == "v2" => UploadApiVersion::V2, - Ok(_) => UploadApiVersion::V1, + Ok(response) if response.api_version == "v2" => { + let answer = message::UploadApiDecision { + version: UploadApiVersion::V2, + inferred_test_collection_short_id: response.test_collection_short_id, + }; + let decision = message::UploadApiDecision { + version: answer.version_for(requested), + inferred_test_collection_short_id: answer + .inferred_for(requested) + .map(String::from), + }; + if decision.version == UploadApiVersion::V1 { + tracing::debug!( + "Trunk chose v2 but named no test collection for this run, using v1" + ); + } + decision + } + Ok(_) => message::UploadApiDecision::default(), Err(error) => { tracing::debug!("Could not choose the Trunk API version, using v1: {error:?}"); - UploadApiVersion::V1 + message::UploadApiDecision::default() } } }; - tracing::info!("Using Trunk API {}", upload_api_version.as_str()); - upload_api_version + tracing::info!("Using Trunk API {}", decision.version.as_str()); + decision } async fn request_upload_api_version( diff --git a/api/src/message.rs b/api/src/message.rs index d4954ad1..e4900820 100644 --- a/api/src/message.rs +++ b/api/src/message.rs @@ -119,6 +119,50 @@ pub struct GetUploadApiVersionRequest { #[serde(rename_all = "camelCase")] pub struct GetUploadApiVersionResponse { pub api_version: String, + /// Sent only with `v2`, for a run that named no collection, when Trunk inferred one from the + /// repository. Older servers never send it. + #[serde(default)] + pub test_collection_short_id: Option, +} + +/// Which API a run uses and, on v2 for a run that named no test collection, the collection Trunk +/// inferred from its repository. +#[derive(Debug, Clone, PartialEq, Eq, Default)] +pub struct UploadApiDecision { + pub version: UploadApiVersion, + pub inferred_test_collection_short_id: Option, +} + +impl UploadApiDecision { + /// The inferred collection, when the run named none of its own and so uses it. + pub fn inferred_for(&self, requested: Option<&str>) -> Option<&str> { + match (self.version, requested.filter(|id| !id.trim().is_empty())) { + (UploadApiVersion::V2, None) => self + .inferred_test_collection_short_id + .as_deref() + .filter(|id| !id.trim().is_empty()), + _ => None, + } + } + + /// Every v2 route is scoped to a collection, so v2 with none named and none inferred is v1. + pub fn version_for(&self, requested: Option<&str>) -> UploadApiVersion { + let has_collection = requested.is_some_and(|id| !id.trim().is_empty()) + || self.inferred_for(requested).is_some(); + match self.version { + UploadApiVersion::V2 if has_collection => UploadApiVersion::V2, + _ => UploadApiVersion::V1, + } + } +} + +impl From for UploadApiDecision { + fn from(version: UploadApiVersion) -> Self { + Self { + version, + inferred_test_collection_short_id: None, + } + } } #[derive(Debug, Serialize, Clone, PartialEq, Eq)] diff --git a/api/tests/v2_client.rs b/api/tests/v2_client.rs index ef9ed528..50835443 100644 --- a/api/tests/v2_client.rs +++ b/api/tests/v2_client.rs @@ -1,7 +1,7 @@ use std::{collections::HashSet, time::Duration}; use api::client::ApiClient; -use api::message::{self, UploadApiCaller, UploadApiVersion}; +use api::message::{self, UploadApiCaller, UploadApiDecision, UploadApiVersion}; use api::quarantine::{QuarantineLookup, QuarantinedTestSet, QuarantinedTests}; use api::urls::TestCaseGuidScope; use axum::{ @@ -131,6 +131,14 @@ fn scope() -> TestCaseGuidScope { } async fn decide(answer: (StatusCode, &'static str)) -> (UploadApiVersion, SharedMockServerState) { + let (decision, state) = decide_for(Some(SHORT_ID), answer).await; + (decision.version, state) +} + +async fn decide_for( + short_id: Option<&str>, + answer: (StatusCode, &'static str), +) -> (UploadApiDecision, SharedMockServerState) { let state = serve(vec![( VERSION_ROUTE, post( @@ -144,10 +152,10 @@ async fn decide(answer: (StatusCode, &'static str)) -> (UploadApiVersion, Shared ), )]) .await; - let version = client(&state, Some("org-key")) - .get_upload_api_version(&version_request(Some(SHORT_ID))) + let decision = client(&state, Some("org-key")) + .get_upload_api_version(&version_request(short_id)) .await; - (version, state) + (decision, state) } fn quarantine_status_route( @@ -279,7 +287,8 @@ async fn retries_a_failed_check_once() { let version = client(&state, Some("org-key")) .get_upload_api_version(&version_request(Some(SHORT_ID))) - .await; + .await + .version; assert_eq!((version, uris(&state).len()), (UploadApiVersion::V2, 2)); } @@ -297,19 +306,20 @@ async fn reads_a_slow_answer_as_v1() { let version = client(&state, Some("org-key")) .get_upload_api_version(&version_request(Some(SHORT_ID))) - .await; + .await + .version; assert_eq!(version, UploadApiVersion::V1); } #[tokio::test] -async fn does_not_ask_without_a_token_or_a_collection() { +async fn does_not_ask_without_a_token_or_a_collection_and_a_repo() { let state = serve(vec![( VERSION_ROUTE, post( |State(state): State, headers: HeaderMap, uri: Uri| async move { state.record_route_request(&headers, &uri, None); - Json(json!({"apiVersion": "v2"})) + Json(json!({"apiVersion": "v2", "testCollectionShortId": SHORT_ID})) }, ), )]) @@ -322,19 +332,84 @@ async fn does_not_ask_without_a_token_or_a_collection() { public_repo_client .get_upload_api_version(&version_request(Some(SHORT_ID))) .await, - UploadApiVersion::V1 + UploadApiDecision::default() ); - for short_id in [None, Some("")] { + // Without a collection, only the repository lets Trunk infer one. + for repo in [ + None, + Some(context::repo::RepoUrlParts { + name: String::new(), + ..repo() + }), + ] { + let request = message::GetUploadApiVersionRequest { + repo, + ..version_request(None) + }; assert_eq!( client(&state, Some("org-key")) - .get_upload_api_version(&version_request(short_id)) + .get_upload_api_version(&request) .await, - UploadApiVersion::V1 + UploadApiDecision::default() ); } assert!(uris(&state).is_empty()); } +#[tokio::test] +async fn uses_the_collection_inferred_for_a_run_without_one() { + for short_id in [None, Some("")] { + let (decision, state) = decide_for( + short_id, + ( + StatusCode::OK, + r#"{"apiVersion":"v2","testCollectionShortId":"zQ8nP2mW","futureField":1}"#, + ), + ) + .await; + + assert_eq!( + decision, + UploadApiDecision { + version: UploadApiVersion::V2, + inferred_test_collection_short_id: Some(String::from("zQ8nP2mW")), + } + ); + assert_eq!( + bodies(&state)[0]["repo"], + json!({"host": "github.com", "owner": "acme", "name": "web"}) + ); + } +} + +#[tokio::test] +async fn reads_v2_without_a_collection_as_v1() { + for answer in [ + r#"{"apiVersion":"v2"}"#, + r#"{"apiVersion":"v2","testCollectionShortId":null}"#, + r#"{"apiVersion":"v2","testCollectionShortId":" "}"#, + r#"{"apiVersion":"v1","testCollectionShortId":"zQ8nP2mW"}"#, + ] { + let (decision, state) = decide_for(None, (StatusCode::OK, answer)).await; + assert_eq!(decision, UploadApiDecision::default(), "{answer}"); + assert_eq!(bodies(&state).len(), 1, "{answer}"); + } +} + +#[tokio::test] +async fn ignores_an_inferred_collection_when_the_run_names_one() { + let (decision, _) = decide_for( + Some(SHORT_ID), + ( + StatusCode::OK, + r#"{"apiVersion":"v2","testCollectionShortId":"zQ8nP2mW"}"#, + ), + ) + .await; + + assert_eq!(decision, UploadApiDecision::from(UploadApiVersion::V2)); +} + #[tokio::test] async fn creates_a_v2_upload() { let state = serve(vec![( diff --git a/cli/src/summary_output/notice.rs b/cli/src/summary_output/notice.rs index 7a6ea973..13fbb645 100644 --- a/cli/src/summary_output/notice.rs +++ b/cli/src/summary_output/notice.rs @@ -11,7 +11,7 @@ pub struct Notice { #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] #[serde(rename_all = "SCREAMING_SNAKE_CASE")] pub enum NoticeKind { - /// A deprecated flag was passed. + /// A deprecated flag was passed, or the run relies on something 1.0.0 removes. Deprecation, /// Part of the Bazel BEP file couldn't be parsed. BepParseError, diff --git a/cli/src/summary_output/text.rs b/cli/src/summary_output/text.rs index f58c71d1..9fea58db 100644 --- a/cli/src/summary_output/text.rs +++ b/cli/src/summary_output/text.rs @@ -181,6 +181,16 @@ pub fn render_upload(report: &UploadReport, options: &TextOptions) -> anyhow::Re output.push(Line::default()); } + // The console shows tracing only when verbose, so a deprecation is shown here or not at all. + for notice in &report.notices { + if notice.kind == NoticeKind::Deprecation { + output.push(Line::from_iter([Span::new_styled_lossy( + style(format!("⚠️ {}", notice.message)).with(Color::Yellow), + )])); + output.push(Line::default()); + } + } + let bold = |text: String| Span::new_styled(style(text).attribute(Attribute::Bold)); let upload_line = match (&report.upload_url, &report.bundle_upload_id) { (Some(url), _) => Some(Line::from_iter([ diff --git a/cli/src/upload_command.rs b/cli/src/upload_command.rs index 6945ba15..278f3401 100644 --- a/cli/src/upload_command.rs +++ b/cli/src/upload_command.rs @@ -4,7 +4,7 @@ use std::path::PathBuf; use std::sync::mpsc::Sender; use api::client::{ApiClient, ApiErrorEndpoint}; -use api::message::{GetUploadApiVersionRequest, UploadApiCaller, UploadApiVersion}; +use api::message::{GetUploadApiVersionRequest, UploadApiCaller, UploadApiDecision}; use api::urls::TestCaseGuidScope; use bundle::{BundleMeta, BundlerUtil, QuarantineResolutionMode, unzip_tarball}; use clap::{ArgAction, Args}; @@ -498,8 +498,8 @@ pub struct RunUploadOptions { Option, pub quarantine_resolution_mode_override: Option, /// The API a caller already chose for this run, so its quarantine lookup and the upload - /// use the same one. `None` asks Trunk. - pub upload_api_version: Option, + /// use the same one (and the same inferred collection). `None` asks Trunk. + pub upload_api_version: Option, pub caller: UploadApiCaller, } @@ -518,7 +518,7 @@ impl Default for RunUploadOptions { } pub async fn run_upload( - upload_args: UploadArgs, + mut upload_args: UploadArgs, options: RunUploadOptions, ) -> anyhow::Result { let RunUploadOptions { @@ -609,10 +609,10 @@ pub async fn run_upload( )? }; - let upload_api_version = match upload_api_version { - Some(upload_api_version) => upload_api_version, + let upload_api_decision = match upload_api_version { + Some(upload_api_decision) => upload_api_decision, // A dry run uploads nothing, so it has nothing to choose. - None if upload_args.dry_run => UploadApiVersion::V1, + None if upload_args.dry_run => UploadApiDecision::default(), None => { api_client .get_upload_api_version(&upload_api_version_request( @@ -624,6 +624,17 @@ pub async fn run_upload( .await } }; + let requested_test_collection_short_id = upload_args.test_collection_short_id.as_deref(); + let upload_api_version = upload_api_decision.version_for(requested_test_collection_short_id); + if let Some(inferred) = upload_api_decision + .inferred_for(requested_test_collection_short_id) + .map(String::from) + { + let message = inferred_test_collection_message(&inferred, caller); + tracing::warn!("{message}"); + notices.push(Notice::deprecation(message)); + upload_args.test_collection_short_id = Some(inferred); + } api_client.set_upload_api_version(upload_api_version); if let Some(bep_result) = &bep_result { @@ -830,6 +841,19 @@ pub async fn run_upload( }) } +/// The run goes on as if the user had named the collection Trunk inferred, until 1.0.0 requires it. +pub fn inferred_test_collection_message(short_id: &str, caller: UploadApiCaller) -> String { + let how_to_pass = match caller { + UploadApiCaller::Cli => format!("Pass --test-collection-id {short_id}"), + UploadApiCaller::TestRunnerPlugin => { + format!("Set {}={short_id}", constants::TRUNK_TEST_COLLECTION_ID_ENV) + } + }; + format!( + "Uploading to test collection {short_id}, inferred from the repository. {how_to_pass}: analytics-cli 1.0.0 requires it." + ) +} + pub fn upload_api_version_request( upload_args: &UploadArgs, repo: &context::repo::RepoUrlParts, diff --git a/cli/tests/summary/upload.rs b/cli/tests/summary/upload.rs index e3558fb9..0e4b35cf 100644 --- a/cli/tests/summary/upload.rs +++ b/cli/tests/summary/upload.rs @@ -226,6 +226,31 @@ fn prints_a_passing_run() { ); } +#[test] +fn prints_deprecation_notices_first() { + let mut result = upload_result(5); + result.notices = vec![ + Notice::deprecation( + "Uploading to test collection zQ8nP2mW, inferred from the repository. Pass --test-collection-id zQ8nP2mW: analytics-cli 1.0.0 requires it.", + ), + Notice::bep_parse_error("not shown here"), + ]; + + assert_eq!( + upload_summary(&result), + [ + "⚠️ Uploading to test collection zQ8nP2mW, inferred from the repository. Pass --test-collection-id zQ8nP2mW: analytics-cli 1.0.0 requires it.", + "", + "🏷️ Bundle Upload ID: 4b1d2c3e-5f60-4a7b-8c9d-0e1f2a3b4c5d", + "", + "📚 Test Report", + " Total: 5 Pass: 5 Fail: 0 Quarantined: 0 Pass Ratio: 100.0%", + "", + "🎉 No test failures found!", + ] + ); +} + #[test] fn prints_that_no_tests_were_found() { let result = upload_result(0); diff --git a/cli/tests/upload.rs b/cli/tests/upload.rs index d23fe922..b4ad49c4 100644 --- a/cli/tests/upload.rs +++ b/cli/tests/upload.rs @@ -3584,35 +3584,36 @@ async fn a_swift_test_xunit_file_reached_twice_is_uploaded_once() { assert_declarations_resolved(cases); } -// NOTE: must be multi threaded to start a mock server -#[tokio::test(flavor = "multi_thread")] -async fn upload_on_v2_when_trunk_chooses_it() { - use axum::routing::post; - use serde_json::{Value, json}; +const V2_UPLOAD_ID: &str = "f47ac10b-58cc-4372-a567-0e02b2c3d479"; +const V2_CREATED_AT: &str = "2026-10-06T16:00:00.000Z"; - const UPLOAD_ID: &str = "f47ac10b-58cc-4372-a567-0e02b2c3d479"; - const CREATED_AT: &str = "2026-10-06T16:00:00.000Z"; +// (route, collection short id, authorization header) per v2 call +type V2Calls = Arc)>>>; - let temp_dir = tempdir().unwrap(); - generate_mock_git_repo(&temp_dir); - generate_mock_valid_junit_xmls_with_failures(&temp_dir); +/// A server whose decision route answers `decision`, and which serves the v2 quarantine lookup +/// and upload, quarantining every failure. +fn v2_mock_server(decision: serde_json::Value) -> (MockServerBuilder, V2Calls) { + use axum::{extract::Path, routing::post}; + use serde_json::{Value, json}; + + const UPLOAD_ID: &str = V2_UPLOAD_ID; + const CREATED_AT: &str = V2_CREATED_AT; - // (route, authorization header) per v2 call - type Calls = Arc)>>>; - let v2_calls: Calls = Arc::default(); + let v2_calls: V2Calls = Arc::default(); let mut mock_server_builder = MockServerBuilder::new(); mock_server_builder.route( "/v1/metrics/getUploadApiVersion", - post(|| async { Json(json!({"apiVersion": "v2"})) }), + post(move || async move { Json(decision) }), ); let calls = v2_calls.clone(); mock_server_builder.route( "/v2/test-collections/:id/quarantine-status", - post(move |headers: axum::http::HeaderMap, Json(body): Json| { + post(move |Path(id): Path, headers: axum::http::HeaderMap, Json(body): Json| { let calls = calls.clone(); async move { calls.lock().unwrap().push(( String::from("quarantine-status"), + id, headers .get("authorization") .and_then(|value| value.to_str().ok()) @@ -3639,11 +3640,14 @@ async fn upload_on_v2_when_trunk_chooses_it() { mock_server_builder.route( "/v2/test-collections/:id/uploads", post( - move |State(state): State, headers: axum::http::HeaderMap| { + move |State(state): State, + Path(id): Path, + headers: axum::http::HeaderMap| { let calls = calls.clone(); async move { calls.lock().unwrap().push(( String::from("uploads"), + id, headers .get("authorization") .and_then(|value| value.to_str().ok()) @@ -3665,29 +3669,23 @@ async fn upload_on_v2_when_trunk_chooses_it() { }, ), ); - let state = mock_server_builder.spawn_mock_server().await; - - CommandBuilder::upload(temp_dir.path(), state.host.clone()) - .command() - .arg("--test-collection-id") - .arg("aB3xY9kQ") - .assert() - // every failure is quarantined - .success(); + (mock_server_builder, v2_calls) +} - assert_eq!( - v2_calls.lock().unwrap().clone(), - vec![ - ( - String::from("quarantine-status"), - Some(String::from("Bearer test-token")) - ), +fn v2_calls_for(short_id: &str) -> Vec<(String, String, Option)> { + ["quarantine-status", "uploads"] + .into_iter() + .map(|route| { ( - String::from("uploads"), - Some(String::from("Bearer test-token")) - ), - ] - ); + String::from(route), + String::from(short_id), + Some(String::from("Bearer test-token")), + ) + }) + .collect() +} + +fn assert_no_v1_calls(state: &SharedMockServerState) { let requests = state.requests.lock().unwrap().clone(); assert!( !requests.iter().any(|request| matches!( @@ -3696,6 +3694,37 @@ async fn upload_on_v2_when_trunk_chooses_it() { )), "a v2 run must not call the v1 routes" ); +} + +const INFERRED_SHORT_ID: &str = "zQ8nP2mW"; + +// NOTE: must be multi threaded to start a mock server +#[tokio::test(flavor = "multi_thread")] +async fn upload_on_v2_when_trunk_chooses_it() { + let temp_dir = tempdir().unwrap(); + generate_mock_git_repo(&temp_dir); + generate_mock_valid_junit_xmls_with_failures(&temp_dir); + + // A run that names its collection ignores any the server infers. + let (mock_server_builder, v2_calls) = v2_mock_server(serde_json::json!({ + "apiVersion": "v2", + "testCollectionShortId": INFERRED_SHORT_ID, + })); + + let state = mock_server_builder.spawn_mock_server().await; + + CommandBuilder::upload(temp_dir.path(), state.host.clone()) + .command() + .arg("--test-collection-id") + .arg("aB3xY9kQ") + .assert() + // every failure is quarantined + .success() + .stderr(predicate::str::contains("inferred from the repository").not()); + + assert_eq!(v2_calls.lock().unwrap().clone(), v2_calls_for("aB3xY9kQ")); + assert_no_v1_calls(&state); + let requests = state.requests.lock().unwrap().clone(); let tar_extract_directory = requests .iter() .find_map(|request| match request { @@ -3712,14 +3741,85 @@ async fn upload_on_v2_when_trunk_chooses_it() { bundle_meta.base_props.test_collection, ), ( - UPLOAD_ID, + V2_UPLOAD_ID, // base64 of {"id":…,"createdAt":…,"repoId":…}, which ingestion decodes "eyJpZCI6ImY0N2FjMTBiLTU4Y2MtNDM3Mi1hNTY3LTBlMDJiMmMzZDQ3OSIsImNyZWF0ZWRBdCI6IjIwMjYtMTAtMDZUMTY6MDA6MDAuMDAwWiIsInJlcG9JZCI6IjdhMWYwZTNkLTJiNGMtNGQ1ZS04ZjkwLTEyMzQ1Njc4OWFiYyJ9", Some(TestCollectionProps { short_id: String::from("aB3xY9kQ"), - bundle_meta_id: String::from(UPLOAD_ID), - bundle_meta_created_at: String::from(CREATED_AT), + bundle_meta_id: String::from(V2_UPLOAD_ID), + bundle_meta_created_at: String::from(V2_CREATED_AT), }), ) ); } + +// NOTE: must be multi threaded to start a mock server +#[tokio::test(flavor = "multi_thread")] +async fn upload_on_v2_to_the_collection_trunk_inferred() { + let temp_dir = tempdir().unwrap(); + generate_mock_git_repo(&temp_dir); + generate_mock_valid_junit_xmls_with_failures(&temp_dir); + + let (mock_server_builder, v2_calls) = v2_mock_server(serde_json::json!({ + "apiVersion": "v2", + "testCollectionShortId": INFERRED_SHORT_ID, + })); + let state = mock_server_builder.spawn_mock_server().await; + + CommandBuilder::upload(temp_dir.path(), state.host.clone()) + .command() + .assert() + // every failure is quarantined, by the inferred collection + .success() + .stderr(predicate::str::contains(format!( + "Uploading to test collection {INFERRED_SHORT_ID}, inferred from the repository. Pass --test-collection-id {INFERRED_SHORT_ID}: analytics-cli 1.0.0 requires it." + ))); + + assert_eq!( + v2_calls.lock().unwrap().clone(), + v2_calls_for(INFERRED_SHORT_ID) + ); + assert_no_v1_calls(&state); + 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 + .test_collection + .map(|collection| collection.short_id), + Some(String::from(INFERRED_SHORT_ID)) + ); +} + +// NOTE: must be multi threaded to start a mock server +#[tokio::test(flavor = "multi_thread")] +async fn upload_on_v1_when_trunk_chooses_v2_but_names_no_collection() { + let temp_dir = tempdir().unwrap(); + generate_mock_git_repo(&temp_dir); + generate_mock_valid_junit_xmls(&temp_dir); + + let (mock_server_builder, v2_calls) = v2_mock_server(serde_json::json!({"apiVersion": "v2"})); + let state = mock_server_builder.spawn_mock_server().await; + + CommandBuilder::upload(temp_dir.path(), state.host.clone()) + .command() + .assert() + .stderr(predicate::str::contains("inferred from the repository").not()); + + assert!(v2_calls.lock().unwrap().is_empty()); + let requests = state.requests.lock().unwrap().clone(); + assert!( + requests + .iter() + .any(|request| matches!(request, RequestPayload::CreateBundleUpload(_))), + "the run must have uploaded on v1" + ); +} diff --git a/test_report/src/report.rs b/test_report/src/report.rs index 95a440a7..17f545cf 100644 --- a/test_report/src/report.rs +++ b/test_report/src/report.rs @@ -9,7 +9,9 @@ use std::{ use api::{ client::ApiClient, - message::{self, QuarantineResolutionMode, UploadApiCaller, UploadApiVersion}, + message::{ + self, QuarantineResolutionMode, UploadApiCaller, UploadApiDecision, UploadApiVersion, + }, quarantine::QuarantinedTests, urls::TestCaseGuidScope, }; @@ -32,7 +34,7 @@ use trunk_analytics_cli::{ context::gather_initial_test_context, error_report::ErrorReport, summary_output::{ - ErrorSummary, Quarantining, UploadCommand, UploadReport, + ErrorSummary, NoticeKind, Quarantining, UploadCommand, UploadReport, json::{self, Report}, text::SummaryFileText, }, @@ -65,6 +67,12 @@ fn upload_api_version_cache_file_path( quarantine_disk_cache_dir().join(format!("upload_api_version_{cache_key}.json")) } +fn requested_test_collection_short_id() -> Option { + env::var(constants::TRUNK_TEST_COLLECTION_ID_ENV) + .ok() + .filter(|id| !id.trim().is_empty()) +} + fn now_secs() -> Option { SystemTime::now() .duration_since(SystemTime::UNIX_EPOCH) @@ -72,25 +80,31 @@ fn now_secs() -> Option { .map(|duration| duration.as_secs()) } -fn load_upload_api_version(cache_path: &Path, ttl_secs: u64) -> Option { +fn load_upload_api_version(cache_path: &Path, ttl_secs: u64) -> Option { let entry: UploadApiVersionDiskCacheEntry = serde_json::from_reader(fs::File::open(cache_path).ok()?).ok()?; if now_secs()?.saturating_sub(entry.cached_at_secs) >= ttl_secs { return None; } - Some(if entry.v2 { - UploadApiVersion::V2 - } else { - UploadApiVersion::V1 + Some(UploadApiDecision { + version: if entry.v2 { + UploadApiVersion::V2 + } else { + UploadApiVersion::V1 + }, + inferred_test_collection_short_id: entry.inferred_test_collection_short_id, }) } -fn save_upload_api_version(cache_path: &Path, upload_api_version: UploadApiVersion) { +fn save_upload_api_version(cache_path: &Path, upload_api_decision: &UploadApiDecision) { let Some(cached_at_secs) = now_secs() else { return; }; let entry = UploadApiVersionDiskCacheEntry { - v2: upload_api_version == UploadApiVersion::V2, + v2: upload_api_decision.version == UploadApiVersion::V2, + inferred_test_collection_short_id: upload_api_decision + .inferred_test_collection_short_id + .clone(), cached_at_secs, }; let written = cache_path @@ -131,6 +145,9 @@ struct QuarantineConfigDiskCacheEntry { #[derive(Debug, Clone, Serialize, Deserialize)] struct UploadApiVersionDiskCacheEntry { v2: bool, + /// The collection Trunk inferred for a run that names none, so every worker uses the same. + #[serde(default, skip_serializing_if = "Option::is_none")] + inferred_test_collection_short_id: Option, cached_at_secs: u64, } @@ -150,7 +167,7 @@ pub struct TestReport { quarantine_config: Option, quarantine_lookup_source: Option, quarantined_tests_disk_cache_ttl: Duration, - upload_api_version: Option, + upload_api_version: Option, codeowners: Option, variant: Option, repo: Option, @@ -490,8 +507,10 @@ impl MutTestReport { ); match (api_client, bundle_repo) { (Ok(mut api_client), Ok(bundle_repo)) => { + let upload_api_decision = self.upload_api_version(&api_client, &bundle_repo.repo); api_client.set_upload_api_version( - self.upload_api_version(&api_client, &bundle_repo.repo), + upload_api_decision + .version_for(requested_test_collection_short_id().as_deref()), ); let variant = env::var(constants::TRUNK_VARIANT_ENV).unwrap_or_else(|_| "".to_string()); @@ -550,10 +569,13 @@ impl MutTestReport { test_collection_short_id: Option<&str>, ) -> PathBuf { // v1 and v2 entries key their tests differently, so they must not share a file. - let version_suffix = match self.0.borrow().upload_api_version { - Some(UploadApiVersion::V2) => "#v2", - _ => "", - }; + let version_suffix = + match self.0.borrow().upload_api_version.as_ref().map(|decision| { + decision.version_for(requested_test_collection_short_id().as_deref()) + }) { + Some(UploadApiVersion::V2) => "#v2", + _ => "", + }; // 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. @@ -674,9 +696,7 @@ impl MutTestReport { return; } - let test_collection_short_id = env::var(constants::TRUNK_TEST_COLLECTION_ID_ENV) - .ok() - .filter(|id| !id.is_empty()); + let test_collection_short_id = self.test_collection_short_id(); if let Some(cache_entry) = self.load_quarantine_config_from_disk_cache( &org_url_slug, @@ -767,9 +787,9 @@ impl MutTestReport { /// Chosen once per run, so the quarantine lookups and the upload use the same API. Kept on /// disk with the quarantine cache's TTL, because a runner may check quarantine in several /// worker processes and upload from another. - fn upload_api_version(&self, api_client: &ApiClient, repo: &RepoUrlParts) -> UploadApiVersion { - if let Some(upload_api_version) = self.0.borrow().upload_api_version { - return upload_api_version; + fn upload_api_version(&self, api_client: &ApiClient, repo: &RepoUrlParts) -> UploadApiDecision { + if let Some(upload_api_decision) = self.0.borrow().upload_api_version.clone() { + return upload_api_decision; } let org_url_slug = self.get_org_url_slug(); let test_collection_short_id = env::var(constants::TRUNK_TEST_COLLECTION_ID_ENV).ok(); @@ -779,9 +799,9 @@ impl MutTestReport { test_collection_short_id.as_deref(), ); let ttl = self.0.borrow().quarantined_tests_disk_cache_ttl.as_secs(); - if let Some(upload_api_version) = load_upload_api_version(&cache_path, ttl) { - self.0.borrow_mut().upload_api_version = Some(upload_api_version); - return upload_api_version; + if let Some(upload_api_decision) = load_upload_api_version(&cache_path, ttl) { + self.0.borrow_mut().upload_api_version = Some(upload_api_decision.clone()); + return upload_api_decision; } let mut upload_args = trunk_analytics_cli::upload_command::UploadArgs::new( self.get_token(), @@ -798,14 +818,28 @@ impl MutTestReport { &envs, UploadApiCaller::TestRunnerPlugin, ); - let upload_api_version = tokio::runtime::Builder::new_multi_thread() + let upload_api_decision = tokio::runtime::Builder::new_multi_thread() .enable_all() .build() .unwrap() .block_on(api_client.get_upload_api_version(&request)); - self.0.borrow_mut().upload_api_version = Some(upload_api_version); - save_upload_api_version(&cache_path, upload_api_version); - upload_api_version + self.0.borrow_mut().upload_api_version = Some(upload_api_decision.clone()); + save_upload_api_version(&cache_path, &upload_api_decision); + upload_api_decision + } + + /// The collection the run's quarantine lookups use: `TRUNK_TEST_COLLECTION_ID`, else the one + /// Trunk inferred from the repository. The upload applies the same rule in `run_upload`. + fn test_collection_short_id(&self) -> Option { + let requested = requested_test_collection_short_id(); + if requested.is_some() { + return requested; + } + self.0 + .borrow() + .upload_api_version + .as_ref() + .and_then(|decision| decision.inferred_for(None).map(String::from)) } fn get_org_url_slug(&self) -> String { @@ -972,12 +1006,19 @@ impl MutTestReport { quarantine_resolution_mode_override: Some( self.quarantine_resolution_mode(), ), - upload_api_version: self.0.borrow().upload_api_version, + upload_api_version: self.0.borrow().upload_api_version.clone(), caller: UploadApiCaller::TestRunnerPlugin, ..Default::default() }, )) { Ok(upload_result) => { + // On stderr, as `write_summary_file` does: the gem's logs reach the + // console only at error level. + for notice in &upload_result.notices { + if notice.kind == NoticeKind::Deprecation { + eprintln!("{}", notice.message); + } + } if let Some(path) = &summary_output_file { let mut report = UploadReport::new(&upload_result, UploadCommand::RSpec); diff --git a/test_report/tests/quarantine.rs b/test_report/tests/quarantine.rs index 3b829bec..5a54f2fd 100644 --- a/test_report/tests/quarantine.rs +++ b/test_report/tests/quarantine.rs @@ -791,6 +791,135 @@ async fn v2_matches_a_test_by_its_computed_test_id() { assert_eq!(results, [(true, false, false), (false, false, false)]); } +#[tokio::test(flavor = "multi_thread")] +#[serial] +async fn v2_uses_the_collection_trunk_inferred_in_every_process() { + use std::sync::{ + Arc, Mutex, + atomic::{AtomicUsize, Ordering}, + }; + + use axum::{extract::Path, routing::post}; + use context::meta::id::gen_test_case_guid; + use serde_json::json; + use uuid::Uuid; + + const INFERRED_SHORT_ID: &str = "zQ8nP2mW"; + const COLLECTION_UUID: &str = "204b6c93-1c51-4e0f-a13c-e07b9bc60688"; + const REPO_ID: &str = "7a1f0e3d-2b4c-4d5e-8f90-123456789abc"; + + cleanup_env_vars(); + let temp_dir = tempdir().unwrap(); + setup_quarantine_disk_cache_dir(&temp_dir); + let _ = env::set_current_dir(&temp_dir); + + let repo = RepoUrlParts { + host: "github.com".into(), + owner: "test-org".into(), + name: "test-repo".into(), + }; + let quarantined_guid = gen_test_case_guid( + Uuid::parse_str(COLLECTION_UUID).unwrap(), + Uuid::parse_str(REPO_ID).unwrap(), + Uuid::parse_str( + &Test::new( + None::, + "quarantined".to_string(), + "test-parent-name".to_string(), + None, + None, + "test-org".to_string(), + &repo, + None, + "".to_string(), + ) + .id, + ) + .unwrap(), + ) + .to_string(); + + let version_calls = Arc::new(AtomicUsize::new(0)); + let listed_collections: Arc>> = Arc::default(); + let state = { + let mut builder = MockServerBuilder::new(); + builder.set_get_quarantining_config_handler(|| async { + (StatusCode::INTERNAL_SERVER_ERROR, "v1 must not be called") + }); + let calls = version_calls.clone(); + builder.route( + "/v1/metrics/getUploadApiVersion", + post(move || { + calls.fetch_add(1, Ordering::SeqCst); + async { + Json(json!({"apiVersion": "v2", "testCollectionShortId": INFERRED_SHORT_ID})) + } + }), + ); + let listed = listed_collections.clone(); + builder.route( + "/v2/test-collections/:id/all-quarantined-tests", + post(move |Path(id): Path| { + listed.lock().unwrap().push(id); + let quarantined_guid = quarantined_guid.clone(); + async move { + Json(json!({ + "data": [{"testId": quarantined_guid}], + "nextCursor": null, + "hasMore": false, + "testCollectionUuid": COLLECTION_UUID, + "repositoryId": REPO_ID, + "quarantineEnabled": true, + })) + } + }), + ); + builder.spawn_mock_server().await + }; + + unsafe { + env::set_var(TRUNK_PUBLIC_API_ADDRESS_ENV, &state.host); + env::set_var(TRUNK_API_TOKEN_ENV, "test-token"); + env::set_var(TRUNK_ORG_URL_SLUG_ENV, "test-org"); + env::set_var( + TRUNK_REPO_URL_ENV, + "https://github.com/test-org/test-repo.git", + ); + env::set_var(TRUNK_USE_UNCLONED_REPO_ENV, "true"); + env::set_var(TRUNK_REPO_HEAD_SHA_ENV, ""); + env::set_var(TRUNK_REPO_HEAD_BRANCH_ENV, ""); + env::set_var(TRUNK_REPO_HEAD_AUTHOR_NAME_ENV, ""); + env::set_var(TRUNK_QUARANTINED_TESTS_DISK_CACHE_TTL_SECS_ENV, "300"); + } + + // Each report stands for a worker process: the second reads the choice, inferred collection + // included, from disk, and must still look up quarantine on v2. + let results: Vec<_> = (0..2) + .map(|_| { + thread::spawn(|| { + let result = MutTestReport::new("test".into(), "test-command".into(), None) + .is_quarantined( + None, + Some("quarantined".to_string()), + Some("test-parent-name".to_string()), + None, + None, + ); + (result.test_is_quarantined, result.quarantine_lookup_failed) + }) + .join() + .unwrap() + }) + .collect(); + + assert_eq!(results, [(true, false), (true, false)]); + assert_eq!(version_calls.load(Ordering::SeqCst), 1); + assert_eq!( + listed_collections.lock().unwrap().clone(), + [INFERRED_SHORT_ID] + ); +} + #[tokio::test(flavor = "multi_thread")] #[serial] async fn shares_the_api_version_choice_across_processes() { From 8f55b6f29169803b32206db328c7481dbe042791 Mon Sep 17 00:00:00 2001 From: Tyler Beebe Date: Wed, 4 Nov 2026 17:39:49 +0000 Subject: [PATCH 2/4] feat(cli): skip the upload when Trunk has no test collection for the run When getUploadApiVersion answers missingTestCollection, the run named no collection and Trunk could not infer one for a fully migrated org, so ingestion would drop a v1 upload. The CLI and the test-runner plugins now skip the upload and its telemetry and warn, with an UPLOAD_SKIPPED notice in the summary. The quarantine lookup is unchanged. Part of TRUNK-23145 Co-Authored-By: Claude Opus 5.5 --- api/src/client/mod.rs | 11 +++++- api/src/message.rs | 8 +++- api/tests/v2_client.rs | 20 ++++++++++ cli/src/summary_output/notice.rs | 9 +++++ cli/src/summary_output/text.rs | 7 +++- cli/src/upload_command.rs | 66 +++++++++++++++++++++++++------- cli/tests/upload.rs | 41 ++++++++++++++++++++ test_report/src/report.rs | 4 ++ 8 files changed, 147 insertions(+), 19 deletions(-) diff --git a/api/src/client/mod.rs b/api/src/client/mod.rs index 2acc5028..c57f35ff 100644 --- a/api/src/client/mod.rs +++ b/api/src/client/mod.rs @@ -238,7 +238,8 @@ impl ApiClient { /// retried once; any other answer is final. /// /// A run that names no collection asks too, by its repository: Trunk may answer `v2` with the - /// collection it inferred. A `v2` that names none is v1, since every v2 route needs one. + /// collection it inferred. A `v2` that names none is v1, since every v2 route needs one. A `v1` + /// with `missingTestCollection` means Trunk has no collection for the run at all. pub async fn get_upload_api_version( &self, request: &message::GetUploadApiVersionRequest, @@ -260,12 +261,14 @@ impl ApiClient { let answer = message::UploadApiDecision { version: UploadApiVersion::V2, inferred_test_collection_short_id: response.test_collection_short_id, + ..message::UploadApiDecision::default() }; let decision = message::UploadApiDecision { version: answer.version_for(requested), inferred_test_collection_short_id: answer .inferred_for(requested) .map(String::from), + ..message::UploadApiDecision::default() }; if decision.version == UploadApiVersion::V1 { tracing::debug!( @@ -274,7 +277,11 @@ impl ApiClient { } decision } - Ok(_) => message::UploadApiDecision::default(), + Ok(response) => message::UploadApiDecision { + missing_test_collection: response.missing_test_collection + && requested.is_none(), + ..message::UploadApiDecision::default() + }, Err(error) => { tracing::debug!("Could not choose the Trunk API version, using v1: {error:?}"); message::UploadApiDecision::default() diff --git a/api/src/message.rs b/api/src/message.rs index e4900820..ec7d51e5 100644 --- a/api/src/message.rs +++ b/api/src/message.rs @@ -123,6 +123,10 @@ pub struct GetUploadApiVersionResponse { /// repository. Older servers never send it. #[serde(default)] pub test_collection_short_id: Option, + /// Sent only with `v1`, for a fully migrated org's run that named no collection when Trunk + /// could not infer one. Trunk would drop a v1 upload of that run. Older servers never send it. + #[serde(default)] + pub missing_test_collection: bool, } /// Which API a run uses and, on v2 for a run that named no test collection, the collection Trunk @@ -131,6 +135,8 @@ pub struct GetUploadApiVersionResponse { pub struct UploadApiDecision { pub version: UploadApiVersion, pub inferred_test_collection_short_id: Option, + /// The run named no collection and Trunk has none for it, so the upload is skipped. + pub missing_test_collection: bool, } impl UploadApiDecision { @@ -160,7 +166,7 @@ impl From for UploadApiDecision { fn from(version: UploadApiVersion) -> Self { Self { version, - inferred_test_collection_short_id: None, + ..Self::default() } } } diff --git a/api/tests/v2_client.rs b/api/tests/v2_client.rs index 50835443..846c47dc 100644 --- a/api/tests/v2_client.rs +++ b/api/tests/v2_client.rs @@ -373,6 +373,7 @@ async fn uses_the_collection_inferred_for_a_run_without_one() { UploadApiDecision { version: UploadApiVersion::V2, inferred_test_collection_short_id: Some(String::from("zQ8nP2mW")), + missing_test_collection: false, } ); assert_eq!( @@ -382,6 +383,25 @@ async fn uses_the_collection_inferred_for_a_run_without_one() { } } +#[tokio::test] +async fn reads_a_missing_collection_for_a_run_without_one() { + let answer = r#"{"apiVersion":"v1","missingTestCollection":true}"#; + for short_id in [None, Some("")] { + let (decision, _) = decide_for(short_id, (StatusCode::OK, answer)).await; + assert_eq!( + decision, + UploadApiDecision { + version: UploadApiVersion::V1, + inferred_test_collection_short_id: None, + missing_test_collection: true, + }, + "{short_id:?}" + ); + } + let (decision, _) = decide_for(Some(SHORT_ID), (StatusCode::OK, answer)).await; + assert_eq!(decision, UploadApiDecision::default()); +} + #[tokio::test] async fn reads_v2_without_a_collection_as_v1() { for answer in [ diff --git a/cli/src/summary_output/notice.rs b/cli/src/summary_output/notice.rs index 13fbb645..8e08af0e 100644 --- a/cli/src/summary_output/notice.rs +++ b/cli/src/summary_output/notice.rs @@ -15,6 +15,8 @@ pub enum NoticeKind { Deprecation, /// Part of the Bazel BEP file couldn't be parsed. BepParseError, + /// The results weren't uploaded: the run named no test collection and Trunk has none for it. + UploadSkipped, } impl Notice { @@ -25,6 +27,13 @@ impl Notice { } } + pub fn upload_skipped>(message: T) -> Self { + Self { + kind: NoticeKind::UploadSkipped, + message: message.into(), + } + } + pub fn bep_parse_error>(message: T) -> Self { Self { kind: NoticeKind::BepParseError, diff --git a/cli/src/summary_output/text.rs b/cli/src/summary_output/text.rs index 9fea58db..b3795556 100644 --- a/cli/src/summary_output/text.rs +++ b/cli/src/summary_output/text.rs @@ -181,9 +181,12 @@ pub fn render_upload(report: &UploadReport, options: &TextOptions) -> anyhow::Re output.push(Line::default()); } - // The console shows tracing only when verbose, so a deprecation is shown here or not at all. + // The console shows tracing only when verbose, so these are shown here or not at all. for notice in &report.notices { - if notice.kind == NoticeKind::Deprecation { + if matches!( + notice.kind, + NoticeKind::Deprecation | NoticeKind::UploadSkipped + ) { output.push(Line::from_iter([Span::new_styled_lossy( style(format!("⚠️ {}", notice.message)).with(Color::Yellow), )])); diff --git a/cli/src/upload_command.rs b/cli/src/upload_command.rs index 278f3401..c59d3880 100644 --- a/cli/src/upload_command.rs +++ b/cli/src/upload_command.rs @@ -626,6 +626,9 @@ pub async fn run_upload( }; let requested_test_collection_short_id = upload_args.test_collection_short_id.as_deref(); let upload_api_version = upload_api_decision.version_for(requested_test_collection_short_id); + let skip_upload = upload_api_decision.missing_test_collection + && requested_test_collection_short_id + .is_none_or(|test_collection_short_id| test_collection_short_id.trim().is_empty()); if let Some(inferred) = upload_api_decision .inferred_for(requested_test_collection_short_id) .map(String::from) @@ -635,6 +638,11 @@ pub async fn run_upload( notices.push(Notice::deprecation(message)); upload_args.test_collection_short_id = Some(inferred); } + if skip_upload { + let message = missing_test_collection_message(&meta.base_props.repo.repo, caller); + tracing::warn!("{message}"); + notices.push(Notice::upload_skipped(message)); + } api_client.set_upload_api_version(upload_api_version); if let Some(bep_result) = &bep_result { @@ -738,16 +746,22 @@ pub async fn run_upload( meta.failed_tests = quarantine_context.failures.clone(); let upload_started_at = chrono::Utc::now(); - tracing::info!("Uploading test results..."); - let upload_bundle_result = upload_bundle( - &mut meta, - upload_args.test_collection_short_id.clone(), - &api_client, - bep_result, - quarantine_context.exit_code, - upload_args.dry_run, - ) - .await; + let upload_bundle_result = if skip_upload { + None + } else { + tracing::info!("Uploading test results..."); + Some( + upload_bundle( + &mut meta, + upload_args.test_collection_short_id.clone(), + &api_client, + bep_result, + quarantine_context.exit_code, + upload_args.dry_run, + ) + .await, + ) + }; let quarantine_query_result = quarantine_query_result_override .unwrap_or_else(|| quarantine_query_result(disable_quarantining, &quarantine_context)); let upload_metrics = proto::upload_metrics::trunk::UploadMetrics { @@ -774,7 +788,9 @@ pub async fn run_upload( .into(), }; let mut request = api::message::TelemetryUploadMetricsRequest { upload_metrics }; - if !upload_args.dry_run { + if !upload_args.dry_run + && let Some(upload_bundle_result) = &upload_bundle_result + { let telemetry_response; if let Some(err) = upload_bundle_result.as_ref().err() { request.upload_metrics.failed = true; @@ -793,11 +809,12 @@ pub async fn run_upload( } } - if upload_bundle_result.is_err() { + if matches!(upload_bundle_result, Some(Err(_))) { tracing::error!("Failed to upload bundle"); } let (guid_scope, error_report) = match upload_bundle_result { - Ok(uploaded) => { + None => (None, None), + Some(Ok(uploaded)) => { if upload_args.dry_run { let curr_dir = env::current_dir()?; let bundle_file = curr_dir.join(DRY_RUN_OUTPUT_DIR); @@ -805,7 +822,7 @@ pub async fn run_upload( } (uploaded.guid_scope, None) } - Err(e) => ( + Some(Err(e)) => ( None, Some(ErrorReport::new( e, @@ -854,6 +871,27 @@ pub fn inferred_test_collection_message(short_id: &str, caller: UploadApiCaller) ) } +/// Trunk would drop the upload, since the org files every upload under a test collection. +pub fn missing_test_collection_message( + repo: &context::repo::RepoUrlParts, + caller: UploadApiCaller, +) -> String { + let how_to_pass = match caller { + UploadApiCaller::Cli => String::from("Pass --test-collection-id"), + UploadApiCaller::TestRunnerPlugin => { + format!("Set {}", constants::TRUNK_TEST_COLLECTION_ID_ENV) + } + }; + let repo_name = if repo.owner.is_empty() && repo.name.is_empty() { + String::from("this repository") + } else { + format!("{}/{}", repo.owner, repo.name) + }; + format!( + "Test results were not uploaded: this run names no test collection, and Trunk has none for {repo_name}. {how_to_pass} to upload them." + ) +} + pub fn upload_api_version_request( upload_args: &UploadArgs, repo: &context::repo::RepoUrlParts, diff --git a/cli/tests/upload.rs b/cli/tests/upload.rs index b4ad49c4..3b59d8af 100644 --- a/cli/tests/upload.rs +++ b/cli/tests/upload.rs @@ -3799,6 +3799,47 @@ async fn upload_on_v2_to_the_collection_trunk_inferred() { ); } +// NOTE: must be multi threaded to start a mock server +#[tokio::test(flavor = "multi_thread")] +async fn skip_the_upload_when_trunk_has_no_collection_for_the_run() { + let temp_dir = tempdir().unwrap(); + generate_mock_git_repo(&temp_dir); + generate_mock_valid_junit_xmls(&temp_dir); + + let (mock_server_builder, v2_calls) = + v2_mock_server(serde_json::json!({"apiVersion": "v1", "missingTestCollection": true})); + let state = mock_server_builder.spawn_mock_server().await; + + let output = CommandBuilder::upload(temp_dir.path(), state.host.clone()) + .command() + .assert() + // The fixture's failures aren't quarantined, so the run fails either way. + .code(1) + .get_output() + .clone(); + + let console = format!( + "{}{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + assert!( + console.contains("Test results were not uploaded"), + "the run must say it skipped the upload: {console}" + ); + assert!(v2_calls.lock().unwrap().is_empty()); + let requests = state.requests.lock().unwrap().clone(); + assert!( + !requests.iter().any(|request| matches!( + request, + RequestPayload::CreateBundleUpload(_) + | RequestPayload::S3Upload(_) + | RequestPayload::TelemetryUploadMetrics(_) + )), + "a skipped upload must not call the upload routes" + ); +} + // NOTE: must be multi threaded to start a mock server #[tokio::test(flavor = "multi_thread")] async fn upload_on_v1_when_trunk_chooses_v2_but_names_no_collection() { diff --git a/test_report/src/report.rs b/test_report/src/report.rs index 17f545cf..5ee65b31 100644 --- a/test_report/src/report.rs +++ b/test_report/src/report.rs @@ -93,6 +93,7 @@ fn load_upload_api_version(cache_path: &Path, ttl_secs: u64) -> Option, + #[serde(default, skip_serializing_if = "std::ops::Not::not")] + missing_test_collection: bool, cached_at_secs: u64, } From abee24755a9b7671aa988158542c2a5c3246b17a Mon Sep 17 00:00:00 2001 From: Tyler Beebe Date: Wed, 4 Nov 2026 17:52:33 +0000 Subject: [PATCH 3/4] refactor(api): read the upload API answer in one place UploadApiDecision::from_response holds the rules: a run that named its collection takes the answer's version as it is, and only a run that named none reads testCollectionShortId (with v2) or missingTestCollection (with v1). Co-Authored-By: Claude Opus 5.5 --- api/src/client/mod.rs | 26 +------------------------- api/src/message.rs | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 25 deletions(-) diff --git a/api/src/client/mod.rs b/api/src/client/mod.rs index c57f35ff..73d98d92 100644 --- a/api/src/client/mod.rs +++ b/api/src/client/mod.rs @@ -257,31 +257,7 @@ impl ApiClient { message::UploadApiDecision::default() } else { match self.request_upload_api_version(request).await { - Ok(response) if response.api_version == "v2" => { - let answer = message::UploadApiDecision { - version: UploadApiVersion::V2, - inferred_test_collection_short_id: response.test_collection_short_id, - ..message::UploadApiDecision::default() - }; - let decision = message::UploadApiDecision { - version: answer.version_for(requested), - inferred_test_collection_short_id: answer - .inferred_for(requested) - .map(String::from), - ..message::UploadApiDecision::default() - }; - if decision.version == UploadApiVersion::V1 { - tracing::debug!( - "Trunk chose v2 but named no test collection for this run, using v1" - ); - } - decision - } - Ok(response) => message::UploadApiDecision { - missing_test_collection: response.missing_test_collection - && requested.is_none(), - ..message::UploadApiDecision::default() - }, + Ok(response) => message::UploadApiDecision::from_response(response, requested), Err(error) => { tracing::debug!("Could not choose the Trunk API version, using v1: {error:?}"); message::UploadApiDecision::default() diff --git a/api/src/message.rs b/api/src/message.rs index ec7d51e5..e0684164 100644 --- a/api/src/message.rs +++ b/api/src/message.rs @@ -140,6 +140,42 @@ pub struct UploadApiDecision { } impl UploadApiDecision { + /// A run that named its collection takes the answer's version as it is. Only a run that named + /// none reads the other fields: the inferred collection with `v2`, `missingTestCollection` with + /// `v1`. + pub fn from_response(response: GetUploadApiVersionResponse, requested: Option<&str>) -> Self { + let v2 = response.api_version == "v2"; + if requested.is_some_and(|id| !id.trim().is_empty()) { + return Self::from(if v2 { + UploadApiVersion::V2 + } else { + UploadApiVersion::V1 + }); + } + if !v2 { + return Self { + missing_test_collection: response.missing_test_collection, + ..Self::default() + }; + } + match response + .test_collection_short_id + .filter(|id| !id.trim().is_empty()) + { + Some(inferred) => Self { + version: UploadApiVersion::V2, + inferred_test_collection_short_id: Some(inferred), + ..Self::default() + }, + None => { + tracing::debug!( + "Trunk chose v2 but named no test collection for this run, using v1" + ); + Self::default() + } + } + } + /// The inferred collection, when the run named none of its own and so uses it. pub fn inferred_for(&self, requested: Option<&str>) -> Option<&str> { match (self.version, requested.filter(|id| !id.trim().is_empty())) { From 9287c00df029caf73363f4195bb1e1f0ff82561d Mon Sep 17 00:00:00 2001 From: Tyler Beebe Date: Wed, 4 Nov 2026 17:58:39 +0000 Subject: [PATCH 4/4] feat(cli): make no v1 calls for a run Trunk has no test collection for With missingTestCollection the CLI now skips the quarantine lookup as well as the upload, and the test-runner plugins report a test as not quarantined without a lookup. v1 answers that lookup for a fully migrated org with 404 "A test collection is required", which quarantined nothing, so the exit code is the same as before. Part of TRUNK-23145 Co-Authored-By: Claude Opus 5.5 --- api/src/client/mod.rs | 12 +++++++ cli/src/context_quarantine.rs | 48 +++++++++++++------------ cli/src/upload_command.rs | 1 + cli/tests/upload.rs | 3 +- test_report/src/report.rs | 6 ++++ test_report/tests/quarantine.rs | 62 +++++++++++++++++++++++++++++++++ 6 files changed, 108 insertions(+), 24 deletions(-) diff --git a/api/src/client/mod.rs b/api/src/client/mod.rs index 73d98d92..7293c81b 100644 --- a/api/src/client/mod.rs +++ b/api/src/client/mod.rs @@ -60,6 +60,7 @@ pub struct ApiClient { org_url_slug: String, render_sender: Option>, upload_api_version: UploadApiVersion, + missing_test_collection: bool, } pub fn get_api_host() -> String { @@ -221,9 +222,20 @@ impl ApiClient { org_url_slug, render_sender, upload_api_version: UploadApiVersion::V1, + missing_test_collection: false, }) } + /// Trunk has no test collection for this run, so it makes no quarantine lookup and no upload: + /// v1 refuses both for a fully migrated org. + pub fn missing_test_collection(&self) -> bool { + self.missing_test_collection + } + + pub fn set_missing_test_collection(&mut self, missing_test_collection: bool) { + self.missing_test_collection = missing_test_collection; + } + pub fn upload_api_version(&self) -> UploadApiVersion { self.upload_api_version } diff --git a/cli/src/context_quarantine.rs b/cli/src/context_quarantine.rs index ea439399..acf74de0 100644 --- a/cli/src/context_quarantine.rs +++ b/cli/src/context_quarantine.rs @@ -342,31 +342,33 @@ pub async fn gather_quarantine_context( }); } - let (quarantine_config, quarantine_fetch_status) = - if !failed_tests_extractor.failed_tests().is_empty() - && file_set_builder - .file_sets() - .iter() - // internal files track quarantine status directly, so we don't need to check them - .any(|file_set| file_set.file_set_type == FileSetType::Junit) - { - tracing::info!("Checking if failed tests can be quarantined"); - match api_client.get_quarantining_config(request).await { - 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) + let (quarantine_config, quarantine_fetch_status) = if api_client.missing_test_collection() { + tracing::debug!("Skipping quarantine check: Trunk has no test collection for this run."); + (None, QuarantineFetchStatus::FetchSkipped) + } else if !failed_tests_extractor.failed_tests().is_empty() + && file_set_builder + .file_sets() + .iter() + // internal files track quarantine status directly, so we don't need to check them + .any(|file_set| file_set.file_set_type == FileSetType::Junit) + { + tracing::info!("Checking if failed tests can be quarantined"); + match api_client.get_quarantining_config(request).await { + 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}"); } - anyhow::Result::Err(error) => (None, QuarantineFetchStatus::FetchFailed(error)), + (Some(response), QuarantineFetchStatus::FetchSucceeded) } - } else { - tracing::debug!("Skipping quarantine check."); - (None, QuarantineFetchStatus::FetchSkipped) - }; + anyhow::Result::Err(error) => (None, QuarantineFetchStatus::FetchFailed(error)), + } + } else { + tracing::debug!("Skipping quarantine check."); + (None, QuarantineFetchStatus::FetchSkipped) + }; let quarantine_resolution_mode = quarantine_config .as_ref() diff --git a/cli/src/upload_command.rs b/cli/src/upload_command.rs index c59d3880..0cace3f7 100644 --- a/cli/src/upload_command.rs +++ b/cli/src/upload_command.rs @@ -644,6 +644,7 @@ pub async fn run_upload( notices.push(Notice::upload_skipped(message)); } api_client.set_upload_api_version(upload_api_version); + api_client.set_missing_test_collection(skip_upload); if let Some(bep_result) = &bep_result { notices.extend(bep_result.errors.iter().map(Notice::bep_parse_error)); diff --git a/cli/tests/upload.rs b/cli/tests/upload.rs index 3b59d8af..ee3730a8 100644 --- a/cli/tests/upload.rs +++ b/cli/tests/upload.rs @@ -3835,8 +3835,9 @@ async fn skip_the_upload_when_trunk_has_no_collection_for_the_run() { RequestPayload::CreateBundleUpload(_) | RequestPayload::S3Upload(_) | RequestPayload::TelemetryUploadMetrics(_) + | RequestPayload::GetQuarantineConfig(_) )), - "a skipped upload must not call the upload routes" + "a run Trunk has no collection for must not call v1" ); } diff --git a/test_report/src/report.rs b/test_report/src/report.rs index 5ee65b31..b070b8d0 100644 --- a/test_report/src/report.rs +++ b/test_report/src/report.rs @@ -512,6 +512,12 @@ impl MutTestReport { match (api_client, bundle_repo) { (Ok(mut api_client), Ok(bundle_repo)) => { let upload_api_decision = self.upload_api_version(&api_client, &bundle_repo.repo); + if upload_api_decision.missing_test_collection { + tracing::debug!( + "Not checking quarantine status: Trunk has no test collection for this run" + ); + return IsQuarantinedResult::default(); + } api_client.set_upload_api_version( upload_api_decision .version_for(requested_test_collection_short_id().as_deref()), diff --git a/test_report/tests/quarantine.rs b/test_report/tests/quarantine.rs index 5a54f2fd..61bd69b8 100644 --- a/test_report/tests/quarantine.rs +++ b/test_report/tests/quarantine.rs @@ -920,6 +920,68 @@ async fn v2_uses_the_collection_trunk_inferred_in_every_process() { ); } +#[tokio::test(flavor = "multi_thread")] +#[serial] +async fn makes_no_quarantine_lookup_when_trunk_has_no_collection() { + use std::sync::{ + Arc, + atomic::{AtomicUsize, Ordering}, + }; + + use axum::routing::post; + use serde_json::json; + + cleanup_env_vars(); + let temp_dir = tempdir().unwrap(); + setup_quarantine_disk_cache_dir(&temp_dir); + let _ = env::set_current_dir(&temp_dir); + + let v1_calls = Arc::new(AtomicUsize::new(0)); + let state = { + let mut builder = MockServerBuilder::new(); + let calls = v1_calls.clone(); + builder.set_get_quarantining_config_handler(move || { + calls.fetch_add(1, Ordering::SeqCst); + async { (StatusCode::NOT_FOUND, "A test collection is required") } + }); + builder.route( + "/v1/metrics/getUploadApiVersion", + post(|| async { Json(json!({"apiVersion": "v1", "missingTestCollection": true})) }), + ); + builder.spawn_mock_server().await + }; + + unsafe { + env::set_var(TRUNK_PUBLIC_API_ADDRESS_ENV, &state.host); + env::set_var(TRUNK_API_TOKEN_ENV, "test-token"); + env::set_var(TRUNK_ORG_URL_SLUG_ENV, "test-org"); + env::set_var( + TRUNK_REPO_URL_ENV, + "https://github.com/test-org/test-repo.git", + ); + env::set_var(TRUNK_USE_UNCLONED_REPO_ENV, "true"); + env::set_var(TRUNK_REPO_HEAD_SHA_ENV, ""); + env::set_var(TRUNK_REPO_HEAD_BRANCH_ENV, ""); + env::set_var(TRUNK_REPO_HEAD_AUTHOR_NAME_ENV, ""); + } + + let result = thread::spawn(|| { + MutTestReport::new("test".into(), "test-command".into(), None).is_quarantined( + None, + Some("quarantined".to_string()), + Some("test-parent-name".to_string()), + None, + None, + ) + }) + .join() + .unwrap(); + + assert!(!result.test_is_quarantined); + assert!(!result.quarantine_lookup_failed); + assert_eq!(v1_calls.load(Ordering::SeqCst), 0); +} + #[tokio::test(flavor = "multi_thread")] #[serial] async fn shares_the_api_version_choice_across_processes() {