From 2b5debb8fc7d047f3f210187c91e99171195eed8 Mon Sep 17 00:00:00 2001 From: Tyler Jang Date: Fri, 21 Aug 2026 16:08:21 +0000 Subject: [PATCH 1/3] feat(upload): link to test collection pages when a collection id is passed When --test-collection-id / TRUNK_TEST_COLLECTION_ID is provided, end-of-run failure links now use the collection short-link form /{org}/flaky-tests/collections/{short_id}/t/{test_case_id}?repo={owner}/{name}; the webapp resolves the repo name to a repo id and redirects to the canonical collection test detail page. No RPC or response changes are needed since the short id is a client-side input. Pass --hide-test-collection-links / TRUNK_HIDE_TEST_COLLECTION_LINKS=true to opt out and keep the legacy /{org}/flaky-tests/test/{test_case_id}?repo=... links; without a collection id the legacy links are unchanged. --- api/src/urls.rs | 104 ++++++++++++++++++++++++---------- cli/src/context.rs | 2 + cli/src/context_quarantine.rs | 71 +++++++++++++---------- cli/src/upload_command.rs | 23 ++++++++ cli/tests/upload.rs | 67 ++++++++++++++++++++++ constants/src/lib.rs | 2 + 6 files changed, 208 insertions(+), 61 deletions(-) diff --git a/api/src/urls.rs b/api/src/urls.rs index 4f1350a1..17548348 100644 --- a/api/src/urls.rs +++ b/api/src/urls.rs @@ -7,9 +7,14 @@ pub fn url_for_test_case( org_url_slug: &String, repo: &RepoUrlParts, test_case: &Test, + test_collection_short_id: Option<&str>, ) -> Result { let mut url = Url::parse(convert_to_app_url(public_api_address).as_str())?; - url.set_path(test_path(org_url_slug, test_case).as_str()); + let path = match test_collection_short_id { + Some(short_id) => collection_test_path(org_url_slug, short_id, test_case), + None => test_path(org_url_slug, test_case), + }; + url.set_path(path.as_str()); url.set_query(Some(repo_query(repo).as_str())); Ok(url.to_string()) } @@ -22,6 +27,15 @@ fn test_path(org_url_slug: &String, test_case: &Test) -> String { format!("{}/flaky-tests/test/{}", org_url_slug, test_case.id) } +// Short-link form: the webapp resolves the repo query param to a repo id and +// redirects to the canonical collections//tests/_ page. +fn collection_test_path(org_url_slug: &String, short_id: &str, test_case: &Test) -> String { + format!( + "{}/flaky-tests/collections/{}/t/{}", + org_url_slug, short_id, test_case.id + ) +} + fn repo_query(repo: &RepoUrlParts) -> String { let value: String = form_urlencoded::byte_serialize(format!("{}/{}", repo.owner, repo.name).as_bytes()) @@ -29,37 +43,65 @@ fn repo_query(repo: &RepoUrlParts) -> String { format!("repo={}", value) } -#[test] -fn test_url_generated() { - let repo = RepoUrlParts { - host: String::from("https://github.com"), - owner: String::from("bad-app"), - name: String::from("ios-app"), - }; +#[cfg(test)] +mod tests { + use super::*; - let test = Test { - name: String::from("can math"), - parent_name: String::from("basic suite"), - class_name: None, - file: None, - id: String::from("c33a7f64-8f3e-5db9-b37b-2ea870d2441b"), - timestamp_millis: None, - is_quarantined: false, - failure_message: None, - variant: None, - }; + fn test_repo() -> RepoUrlParts { + RepoUrlParts { + host: String::from("https://github.com"), + owner: String::from("bad-app"), + name: String::from("ios-app"), + } + } + + fn test_case() -> Test { + Test { + name: String::from("can math"), + parent_name: String::from("basic suite"), + class_name: None, + file: None, + id: String::from("c33a7f64-8f3e-5db9-b37b-2ea870d2441b"), + timestamp_millis: None, + is_quarantined: false, + failure_message: None, + variant: None, + } + } + + #[test] + fn test_url_generated() { + let actual = url_for_test_case( + &String::from("https://api.trunk-staging.io"), + &String::from("bad-app-org"), + &test_repo(), + &test_case(), + None, + ); + + assert_eq!( + actual, + Ok(String::from( + "https://app.trunk-staging.io/bad-app-org/flaky-tests/test/c33a7f64-8f3e-5db9-b37b-2ea870d2441b?repo=bad-app%2Fios-app" + )), + ); + } - let actual = url_for_test_case( - &String::from("https://api.trunk-staging.io"), - &String::from("bad-app-org"), - &repo, - &test, - ); + #[test] + fn test_collection_url_generated() { + let actual = url_for_test_case( + &String::from("https://api.trunk-staging.io"), + &String::from("bad-app-org"), + &test_repo(), + &test_case(), + Some("tc_123"), + ); - assert_eq!( - actual, - Ok(String::from( - "https://app.trunk-staging.io/bad-app-org/flaky-tests/test/c33a7f64-8f3e-5db9-b37b-2ea870d2441b?repo=bad-app%2Fios-app" - )), - ); + assert_eq!( + actual, + Ok(String::from( + "https://app.trunk-staging.io/bad-app-org/flaky-tests/collections/tc_123/t/c33a7f64-8f3e-5db9-b37b-2ea870d2441b?repo=bad-app%2Fios-app" + )), + ); + } } diff --git a/cli/src/context.rs b/cli/src/context.rs index 1e5da157..4363f3d2 100644 --- a/cli/src/context.rs +++ b/cli/src/context.rs @@ -772,6 +772,7 @@ pub async fn gather_exit_code_and_quarantined_tests_context( file_set_builder: &FileSetBuilder, default_exit_code: Option, test_collection_short_id: Option, + hide_test_collection_links: bool, ) -> anyhow::Result { // Run the quarantine step and update the exit code. let failed_tests_extractor = FailedTestsExtractor::new( @@ -823,6 +824,7 @@ pub async fn gather_exit_code_and_quarantined_tests_context( Some(failed_tests_extractor), default_exit_code, &meta.variant.clone().unwrap_or(String::from("")), + hide_test_collection_links, ) .await? }; diff --git a/cli/src/context_quarantine.rs b/cli/src/context_quarantine.rs index 9bb400e6..48c23619 100644 --- a/cli/src/context_quarantine.rs +++ b/cli/src/context_quarantine.rs @@ -304,6 +304,7 @@ pub async fn gather_quarantine_context( failed_tests_extractor: Option, test_run_exit_code: Option, variant: &String, + hide_test_collection_links: bool, ) -> anyhow::Result { let failed_tests_extractor = failed_tests_extractor.unwrap_or_else(|| { FailedTestsExtractor::new( @@ -329,32 +330,31 @@ 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}"); + 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) } - (Some(response), QuarantineFetchStatus::FetchSucceeded) + anyhow::Result::Err(error) => (None, QuarantineFetchStatus::FetchFailed(error)), } - anyhow::Result::Err(error) => (None, QuarantineFetchStatus::FetchFailed(error)), - } - } else { - tracing::debug!("Skipping quarantine check."); - (None, QuarantineFetchStatus::FetchSkipped) - }; + } else { + tracing::debug!("Skipping quarantine check."); + (None, QuarantineFetchStatus::FetchSkipped) + }; let quarantine_resolution_mode = quarantine_config .as_ref() @@ -413,9 +413,14 @@ pub async fn gather_quarantine_context( quarantined_failures.len(), pluralize("failure", quarantined_failures.len() as isize, false), ); - quarantined_failures - .iter() - .for_each(|quarantined_failure| log_failure(quarantined_failure, request, api_client)); + quarantined_failures.iter().for_each(|quarantined_failure| { + log_failure( + quarantined_failure, + request, + api_client, + hide_test_collection_links, + ) + }); } if !failures.is_empty() { @@ -424,9 +429,9 @@ pub async fn gather_quarantine_context( failures.len(), pluralize("failure", quarantined_failures.len() as isize, false), ); - failures - .iter() - .for_each(|failure| log_failure(failure, request, api_client)); + failures.iter().for_each(|failure| { + log_failure(failure, request, api_client, hide_test_collection_links) + }); } let quarantined_failure_count = quarantined_failures.len(); quarantine_results.quarantine_results = quarantined_failures; @@ -473,12 +478,18 @@ fn log_failure( failure: &Test, request: &api::message::GetQuarantineConfigRequest, api_client: &ApiClient, + hide_test_collection_links: bool, ) { + let test_collection_short_id = request + .test_collection_short_id + .as_deref() + .filter(|_| !hide_test_collection_links); let url = match url_for_test_case( &api_client.api_host, &request.org_url_slug, &request.repo, failure, + test_collection_short_id, ) { Ok(url) => format!("Learn more > {}", url), Err(_) => String::from(""), diff --git a/cli/src/upload_command.rs b/cli/src/upload_command.rs index 426dca44..02c7ce36 100644 --- a/cli/src/upload_command.rs +++ b/cli/src/upload_command.rs @@ -292,6 +292,16 @@ pub struct UploadArgs { default_missing_value = "true" )] pub show_failure_messages: bool, + #[arg( + long, + env = constants::TRUNK_HIDE_TEST_COLLECTION_LINKS_ENV, + help = "Show repo-scoped links in the CLI output instead of test collection links when a test collection ID is passed.", + required = false, + num_args = 0, + default_value = "false", + default_missing_value = "true" + )] + pub hide_test_collection_links: bool, } #[derive(clap::ValueEnum, Clone, Copy, Debug, Default, PartialEq, Eq)] @@ -372,6 +382,8 @@ pub struct UploadRunResult { pub validations: JunitReportValidations, pub validation_report: ValidationReport, pub show_failure_messages: bool, + pub test_collection_short_id: Option, + pub hide_test_collection_links: bool, } pub struct RunUploadOptions { @@ -490,6 +502,7 @@ pub async fn run_upload( .test_collection_short_id .clone() .filter(|id| !id.is_empty()), + upload_args.hide_test_collection_links, ) .await { @@ -641,6 +654,10 @@ pub async fn run_upload( validations, validation_report: upload_args.validation_report, show_failure_messages: upload_args.show_failure_messages, + test_collection_short_id: upload_args + .test_collection_short_id + .filter(|id| !id.is_empty()), + hide_test_collection_links: upload_args.hide_test_collection_links, }) } @@ -771,6 +788,11 @@ impl EndOutput for UploadRunResult { let non_quarantined_count = failures.len(); let all_quarantined = non_quarantined_count == 0 && quarantined_count > 0; + let test_collection_short_id = self + .test_collection_short_id + .as_deref() + .filter(|_| !self.hide_test_collection_links); + // Helper closure to render the test table let render_test_table = |tests: &[Test]| -> anyhow::Result { use std::collections::BTreeMap; @@ -818,6 +840,7 @@ impl EndOutput for UploadRunResult { &self.quarantine_context.org_url_slug, &self.quarantine_context.repo, test, + test_collection_short_id, )?; let mut link_output = Line::from_iter([ Span::new_unstyled("⤷ ")?, diff --git a/cli/tests/upload.rs b/cli/tests/upload.rs index 072c67ab..8712dfe4 100644 --- a/cli/tests/upload.rs +++ b/cli/tests/upload.rs @@ -299,6 +299,73 @@ async fn upload_bundle() { )); } +// NOTE: must be multi threaded to start a mock server +#[tokio::test(flavor = "multi_thread")] +async fn upload_bundle_prints_test_collection_links() { + let temp_dir = tempdir().unwrap(); + generate_mock_git_repo(&temp_dir); + generate_mock_valid_junit_xmls(&temp_dir); + + let state = MockServerBuilder::new().spawn_mock_server().await; + + let assert = CommandBuilder::upload(temp_dir.path(), state.host.clone()) + .command() + .arg("--test-collection-id") + .arg("tc_123") + .assert() + .failure(); + + assert + .stderr(predicate::str::contains( + "/test-org/flaky-tests/collections/tc_123/t/", + )) + .stderr(predicate::str::contains("?repo=trunk-io%2Fanalytics-cli")); +} + +// NOTE: must be multi threaded to start a mock server +#[tokio::test(flavor = "multi_thread")] +async fn upload_bundle_hides_test_collection_links_when_env_set() { + let temp_dir = tempdir().unwrap(); + generate_mock_git_repo(&temp_dir); + generate_mock_valid_junit_xmls(&temp_dir); + + let state = MockServerBuilder::new().spawn_mock_server().await; + + let assert = CommandBuilder::upload(temp_dir.path(), state.host.clone()) + .command() + .env("TRUNK_HIDE_TEST_COLLECTION_LINKS", "true") + .arg("--test-collection-id") + .arg("tc_123") + .assert() + .failure(); + + assert + .stderr(predicate::str::contains("/test-org/flaky-tests/test/")) + .stderr(predicate::str::contains("/flaky-tests/collections/").not()); +} + +// NOTE: must be multi threaded to start a mock server +#[tokio::test(flavor = "multi_thread")] +async fn upload_bundle_without_test_collection_id_prints_repo_links() { + let temp_dir = tempdir().unwrap(); + generate_mock_git_repo(&temp_dir); + generate_mock_valid_junit_xmls(&temp_dir); + + let state = MockServerBuilder::new().spawn_mock_server().await; + + let assert = CommandBuilder::upload(temp_dir.path(), state.host.clone()) + .command() + // an exported-but-blank env var (common in CI) must not produce a + // malformed /collections//t/ link + .env("TRUNK_TEST_COLLECTION_ID", "") + .assert() + .failure(); + + assert + .stderr(predicate::str::contains("/test-org/flaky-tests/test/")) + .stderr(predicate::str::contains("/flaky-tests/collections/").not()); +} + // NOTE: must be multi threaded to start a mock server #[tokio::test(flavor = "multi_thread")] async fn upload_bundle_records_quarantine_resolution_mode() { diff --git a/constants/src/lib.rs b/constants/src/lib.rs index a03b0e66..a2afd054 100644 --- a/constants/src/lib.rs +++ b/constants/src/lib.rs @@ -55,6 +55,7 @@ pub const TRUNK_DRY_RUN_ENV: &str = "TRUNK_DRY_RUN"; pub const TRUNK_TEST_PROCESS_EXIT_CODE_ENV: &str = "TRUNK_TEST_PROCESS_EXIT_CODE"; pub const TRUNK_VALIDATION_REPORT_ENV: &str = "TRUNK_VALIDATION_REPORT"; pub const TRUNK_SHOW_FAILURE_MESSAGES_ENV: &str = "TRUNK_SHOW_FAILURE_MESSAGES"; +pub const TRUNK_HIDE_TEST_COLLECTION_LINKS_ENV: &str = "TRUNK_HIDE_TEST_COLLECTION_LINKS"; pub const TRUNK_DEBUG_ENV: &str = "TRUNK_DEBUG"; // RSpec-only: when set to "true", aborts the RSpec run if quarantine lookup fails. // Handled in rspec-trunk-flaky-tests/lib/trunk_spec_helper.rb, not the CLI. @@ -89,6 +90,7 @@ pub const TRUNK_ENVS_TO_CAPTURE: &[&str] = &[ TRUNK_TEST_PROCESS_EXIT_CODE_ENV, TRUNK_VALIDATION_REPORT_ENV, TRUNK_SHOW_FAILURE_MESSAGES_ENV, + TRUNK_HIDE_TEST_COLLECTION_LINKS_ENV, TRUNK_DEBUG_ENV, ]; From 52d7ddd8025e09139093981adb71fc626d2c272f Mon Sep 17 00:00:00 2001 From: Tyler Jang Date: Fri, 21 Aug 2026 18:54:59 +0000 Subject: [PATCH 2/3] DONOTLAND: intentional test failure to preview collection links in CI Revert this commit before merging. The failing test makes CI's self-upload (built CLI + --test-collection-id) print the new collection short link in the end-of-run output so we can click through it. --- api/src/urls.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/api/src/urls.rs b/api/src/urls.rs index 17548348..17d91dd4 100644 --- a/api/src/urls.rs +++ b/api/src/urls.rs @@ -87,6 +87,12 @@ mod tests { ); } + // DONOTLAND: intentional failure so CI's self-upload prints the new collection link — revert before merge + #[test] + fn donotland_intentional_failure_to_preview_collection_link() { + panic!("DONOTLAND: intentional failure to preview the test collection link in CI output"); + } + #[test] fn test_collection_url_generated() { let actual = url_for_test_case( From 0a36b34cb32fd4f523714e088173ae35d922d02b Mon Sep 17 00:00:00 2001 From: Tyler Jang Date: Fri, 21 Aug 2026 19:31:05 +0000 Subject: [PATCH 3/3] Revert "DONOTLAND: intentional test failure to preview collection links in CI" This reverts commit 52d7ddd8025e09139093981adb71fc626d2c272f. --- api/src/urls.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/api/src/urls.rs b/api/src/urls.rs index 17d91dd4..17548348 100644 --- a/api/src/urls.rs +++ b/api/src/urls.rs @@ -87,12 +87,6 @@ mod tests { ); } - // DONOTLAND: intentional failure so CI's self-upload prints the new collection link — revert before merge - #[test] - fn donotland_intentional_failure_to_preview_collection_link() { - panic!("DONOTLAND: intentional failure to preview the test collection link in CI output"); - } - #[test] fn test_collection_url_generated() { let actual = url_for_test_case(