diff --git a/Cargo.lock b/Cargo.lock index 75b576d9..5bb0f909 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -131,6 +131,7 @@ dependencies = [ "tokio-retry", "tracing", "url", + "uuid", ] [[package]] diff --git a/api/Cargo.toml b/api/Cargo.toml index 6726a9d2..4d40eb0a 100644 --- a/api/Cargo.toml +++ b/api/Cargo.toml @@ -27,6 +27,7 @@ tokio-retry = { version = "0.3", default-features = false } constants = { version = "0.0.0", path = "../constants" } tracing = "0.1.41" url = "2.5.4" +uuid = "1.10.0" proto = { version = "0.0.0", path = "../proto" } prost = "0.12.6" diff --git a/api/src/message.rs b/api/src/message.rs index 9539908b..04a430e5 100644 --- a/api/src/message.rs +++ b/api/src/message.rs @@ -23,6 +23,10 @@ pub struct CreateBundleUploadResponse { pub key: String, pub test_collection_bundle_meta_id: Option, pub test_collection_bundle_meta_created_at: Option, + #[serde(default)] + pub repo_id: Option, + #[serde(default)] + pub test_collection_id: Option, } #[derive(Debug, Serialize, Clone, Deserialize, Default)] diff --git a/api/src/urls.rs b/api/src/urls.rs index 0d024b2a..b1900539 100644 --- a/api/src/urls.rs +++ b/api/src/urls.rs @@ -2,9 +2,17 @@ use anyhow::Context; use base64::{Engine as _, engine::general_purpose::URL_SAFE_NO_PAD}; use bundle::Test; use chrono::DateTime; -use context::repo::RepoUrlParts; +use context::{meta::id::gen_test_case_guid, repo::RepoUrlParts}; use serde::Serialize; use url::{ParseError, Url, form_urlencoded}; +use uuid::Uuid; + +/// The server-resolved half of the test-case GUID tuple, from `CreateBundleUploadResponse`. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct TestCaseGuidScope { + pub test_collection_id: String, + pub repo_id: String, +} pub fn url_for_test_case( public_api_address: &str, @@ -12,8 +20,21 @@ pub fn url_for_test_case( repo: &RepoUrlParts, test_case: &Test, test_collection_short_id: Option<&str>, + guid_scope: Option<&TestCaseGuidScope>, ) -> Result { let mut url = Url::parse(convert_to_app_url(public_api_address).as_str())?; + + let guid_path = match (test_collection_short_id, guid_scope) { + (Some(short_id), Some(ids)) => test_case_guid(ids, test_case) + .map(|guid| collection_test_guid_path(org_url_slug, short_id, &guid)), + _ => None, + }; + // No `?repo=`: the GUID resolves the whole identity tuple, unlike both short-link forms. + if let Some(path) = guid_path { + url.set_path(path.as_str()); + return Ok(url.to_string()); + } + 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), @@ -23,6 +44,14 @@ pub fn url_for_test_case( Ok(url.to_string()) } +fn test_case_guid(ids: &TestCaseGuidScope, test_case: &Test) -> Option { + Some(gen_test_case_guid( + Uuid::parse_str(&ids.test_collection_id).ok()?, + Uuid::parse_str(&ids.repo_id).ok()?, + Uuid::parse_str(&test_case.id).ok()?, + )) +} + /// Serialized to match trunk2's `encodeBundleMetaKey` byte for byte — hence the field order /// and a `Serialize` struct rather than a `format!`. #[derive(Serialize)] @@ -69,6 +98,10 @@ fn test_path(org_url_slug: &String, test_case: &Test) -> String { format!("{}/flaky-tests/test/{}", org_url_slug, test_case.id) } +fn collection_test_guid_path(org_url_slug: &str, short_id: &str, guid: &Uuid) -> String { + format!("{org_url_slug}/flaky-tests/collections/{short_id}/tests/{guid}") +} + // 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 { @@ -98,12 +131,16 @@ mod tests { } fn test_case() -> Test { + test_case_with_id("c33a7f64-8f3e-5db9-b37b-2ea870d2441b") + } + + fn test_case_with_id(id: &str) -> 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"), + id: String::from(id), timestamp_millis: None, is_quarantined: false, failure_message: None, @@ -111,18 +148,35 @@ mod tests { } } - #[test] - fn test_url_generated() { - let actual = url_for_test_case( + const COLLECTION_ID: &str = "018f6d3a-6f2e-4c4a-9b1e-2f3a4b5c6d7e"; + const REPO_ID: &str = "7a1f0e3d-2b4c-4d5e-8f90-123456789abc"; + + fn test_guid_scope() -> TestCaseGuidScope { + TestCaseGuidScope { + test_collection_id: String::from(COLLECTION_ID), + repo_id: String::from(REPO_ID), + } + } + + fn test_case_url( + test_case: &Test, + short_id: Option<&str>, + guid_scope: Option<&TestCaseGuidScope>, + ) -> Result { + url_for_test_case( &String::from("https://api.trunk-staging.io"), &String::from("bad-app-org"), &test_repo(), - &test_case(), - None, - ); + test_case, + short_id, + guid_scope, + ) + } + #[test] + fn test_url_generated() { assert_eq!( - actual, + test_case_url(&test_case(), None, None), Ok(String::from( "https://app.trunk-staging.io/bad-app-org/flaky-tests/test/c33a7f64-8f3e-5db9-b37b-2ea870d2441b?repo=bad-app%2Fios-app" )), @@ -173,19 +227,44 @@ mod tests { #[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!( + test_case_url(&test_case(), Some("tc_123"), None), + 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" + )), ); + } + + #[test] + fn test_collection_guid_url_generated() { + let test_case = test_case_with_id("88e5353c-190c-5dce-9d06-0e66c3e062b1"); assert_eq!( - actual, + test_case_url(&test_case, Some("tc_123"), Some(&test_guid_scope())), 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" + "https://app.trunk-staging.io/bad-app-org/flaky-tests/collections/tc_123/tests/bfeebcf4-72d1-887d-8bcd-788d0dec7f97" )), ); } + + #[test] + fn test_collection_guid_url_falls_back_for_an_unparseable_server_id() { + let ids = TestCaseGuidScope { + test_collection_id: String::from("tc_123"), + repo_id: String::from(REPO_ID), + }; + + assert_eq!( + test_case_url(&test_case(), Some("tc_123"), Some(&ids)), + test_case_url(&test_case(), Some("tc_123"), None), + ); + } + + #[test] + fn test_repo_scoped_url_ignores_the_guid_scope() { + assert_eq!( + test_case_url(&test_case(), None, Some(&test_guid_scope())), + test_case_url(&test_case(), None, None), + ); + } } diff --git a/cli/src/context_quarantine.rs b/cli/src/context_quarantine.rs index 48c23619..6b4fd7a7 100644 --- a/cli/src/context_quarantine.rs +++ b/cli/src/context_quarantine.rs @@ -484,12 +484,14 @@ fn log_failure( .test_collection_short_id .as_deref() .filter(|_| !hide_test_collection_links); + // createBundleUpload has not run yet, so there are no ids to mint a GUID from. let url = match url_for_test_case( &api_client.api_host, &request.org_url_slug, &request.repo, failure, test_collection_short_id, + None, ) { Ok(url) => format!("Learn more > {}", url), Err(_) => String::from(""), diff --git a/cli/src/upload_command.rs b/cli/src/upload_command.rs index e84097c1..05e95448 100644 --- a/cli/src/upload_command.rs +++ b/cli/src/upload_command.rs @@ -4,10 +4,7 @@ use std::path::PathBuf; use std::sync::mpsc::Sender; use api::client::{ApiClient, ApiErrorEndpoint}; -use api::{ - client::get_api_host, - urls::{url_for_test_case, url_for_upload}, -}; +use api::urls::{TestCaseGuidScope, url_for_test_case, url_for_upload}; use bundle::{BundleMeta, BundlerUtil, QuarantineResolutionMode, Test, unzip_tarball}; use clap::{ArgAction, Args}; use codeowners::OwnersSource; @@ -388,6 +385,7 @@ pub struct UploadRunResult { pub test_collection_short_id: Option, pub hide_test_collection_links: bool, pub api_address: String, + pub guid_scope: Option, } pub struct RunUploadOptions { @@ -636,21 +634,32 @@ pub async fn run_upload( if upload_bundle_result.is_err() { tracing::error!("Failed to upload bundle"); } - let error_report = match upload_bundle_result { - Ok(upload_bundle_result) => { + let (guid_scope, error_report) = match upload_bundle_result { + Ok(uploaded) => { if upload_args.dry_run { let curr_dir = env::current_dir()?; let bundle_file = curr_dir.join(DRY_RUN_OUTPUT_DIR); - unzip_tarball(&upload_bundle_result.0, &bundle_file)?; + unzip_tarball(&uploaded.tarball, &bundle_file)?; } - None + (uploaded.guid_scope, None) } - Err(e) => Some(ErrorReport::new( - e, - upload_args.org_url_slug.clone(), - Some("There was an unexpected error that occurred while uploading test results".into()), - )), + Err(e) => ( + None, + Some(ErrorReport::new( + e, + upload_args.org_url_slug.clone(), + Some( + "There was an unexpected error that occurred while uploading test results" + .into(), + ), + )), + ), }; + if guid_scope.is_none() && upload_args.test_collection_short_id.is_some() { + tracing::debug!( + "No test collection ids returned for this upload; test links will use the short-link form" + ); + } Ok(UploadRunResult { quarantine_context, error_report, @@ -663,9 +672,17 @@ pub async fn run_upload( .filter(|id| !id.is_empty()), hide_test_collection_links: upload_args.hide_test_collection_links, api_address: api_client.api_host.clone(), + guid_scope, }) } +struct UploadedBundle { + tarball: PathBuf, + // directory is removed on drop + _temp_dir: TempDir, + guid_scope: Option, +} + async fn upload_bundle( meta: &mut BundleMeta, requested_test_collection_short_id: Option, @@ -673,7 +690,7 @@ async fn upload_bundle( bep_result: Option, exit_code: i32, dry_run: bool, -) -> anyhow::Result<(PathBuf, TempDir)> { +) -> anyhow::Result { let upload_result = gather_upload_id_context( meta, requested_test_collection_short_id, @@ -691,7 +708,11 @@ async fn upload_bundle( if dry_run { tracing::info!("Dry run enabled, not uploading bundle to S3"); - return Ok((bundle_temp_file, bundle_temp_dir)); + return Ok(UploadedBundle { + tarball: bundle_temp_file, + _temp_dir: bundle_temp_dir, + guid_scope: None, + }); } match upload_result { @@ -708,7 +729,16 @@ async fn upload_bundle( ); } - Ok((bundle_temp_file, bundle_temp_dir)) + Ok(UploadedBundle { + tarball: bundle_temp_file, + _temp_dir: bundle_temp_dir, + guid_scope: upload.test_collection_id.zip(upload.repo_id).map( + |(test_collection_id, repo_id)| TestCaseGuidScope { + test_collection_id, + repo_id, + }, + ), + }) } Err(e) => { tracing::error!("Failed to gather upload ID: {}", e); @@ -828,6 +858,10 @@ impl EndOutput for UploadRunResult { .test_collection_short_id .as_deref() .filter(|_| !self.hide_test_collection_links); + let guid_scope = self + .guid_scope + .as_ref() + .filter(|_| !self.hide_test_collection_links); // Helper closure to render the test table let render_test_table = |tests: &[Test]| -> anyhow::Result { @@ -872,11 +906,12 @@ impl EndOutput for UploadRunResult { test_line.pad_left(2); output.push(test_line); let link = url_for_test_case( - &get_api_host(), + &self.api_address, &self.quarantine_context.org_url_slug, &self.quarantine_context.repo, test, test_collection_short_id, + guid_scope, )?; let mut link_output = Line::from_iter([ Span::new_unstyled("⤷ ")?, diff --git a/cli/tests/test.rs b/cli/tests/test.rs index 7d163b80..5901b164 100644 --- a/cli/tests/test.rs +++ b/cli/tests/test.rs @@ -22,7 +22,9 @@ use common::{ use context::{bazel_bep::parser::BazelBepParser, junit::parser::JunitParser}; use predicates::prelude::*; use tempfile::tempdir; -use test_utils::mock_server::{MockServerBuilder, RequestPayload, SharedMockServerState}; +use test_utils::mock_server::{ + MOCK_REPO_ID, MOCK_TEST_COLLECTION_ID, MockServerBuilder, RequestPayload, SharedMockServerState, +}; // NOTE: must be multi threaded to start a mock server #[tokio::test(flavor = "multi_thread")] @@ -308,6 +310,8 @@ async fn quarantining_resets_fail_code() { test_collection_bundle_meta_created_at: Some(String::from( "2026-05-10T12:34:56.000Z", )), + repo_id: Some(String::from(MOCK_REPO_ID)), + test_collection_id: Some(String::from(MOCK_TEST_COLLECTION_ID)), })) } }, @@ -369,6 +373,8 @@ async fn quarantining_not_active_when_disable_quarantining_set() { test_collection_bundle_meta_created_at: Some(String::from( "2026-05-10T12:34:56.000Z", )), + repo_id: Some(String::from(MOCK_REPO_ID)), + test_collection_id: Some(String::from(MOCK_TEST_COLLECTION_ID)), })) } }, @@ -431,6 +437,8 @@ async fn quarantining_not_active_when_disable_true_but_use_true() { test_collection_bundle_meta_created_at: Some(String::from( "2026-05-10T12:34:56.000Z", )), + repo_id: Some(String::from(MOCK_REPO_ID)), + test_collection_id: Some(String::from(MOCK_TEST_COLLECTION_ID)), })) } }, diff --git a/cli/tests/upload.rs b/cli/tests/upload.rs index d4088329..3e3d4f40 100644 --- a/cli/tests/upload.rs +++ b/cli/tests/upload.rs @@ -55,7 +55,10 @@ use tempfile::tempdir; use test_utils::inputs::unpack_archive_to_dir; use test_utils::{ inputs::get_test_file_path, - mock_server::{MockServerBuilder, RequestPayload, SharedMockServerState}, + mock_server::{ + MOCK_REPO_ID, MOCK_TEST_COLLECTION_ID, MockServerBuilder, RequestPayload, + SharedMockServerState, + }, }; use trunk_analytics_cli::upload_command::{DRY_RUN_OUTPUT_DIR, get_bundle_upload_id_message}; @@ -321,6 +324,50 @@ async fn upload_bundle_prints_test_collection_links() { 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/tests/", + )) + .stderr(predicate::str::contains("/collections/tc_123/t/").not()) + .stderr(predicate::str::contains("?repo=trunk-io%2Fanalytics-cli").not()); +} + +// NOTE: must be multi threaded to start a mock server +#[tokio::test(flavor = "multi_thread")] +async fn upload_bundle_falls_back_to_short_links_without_collection_ids() { + let temp_dir = tempdir().unwrap(); + generate_mock_git_repo(&temp_dir); + generate_mock_valid_junit_xmls(&temp_dir); + + async fn create_bundle_without_collection_ids( + State(state): State, + ) -> Json { + let host = &state.host; + Json(CreateBundleUploadResponse { + id: String::from("test-bundle-upload-id"), + id_v2: String::from("test-bundle-upload-id-v2"), + url: format!("{host}/s3upload"), + key: String::from("unused"), + test_collection_bundle_meta_id: Some(String::from( + "82c6a6e5-f8ea-4d93-9a26-b8ab6ff8f6bc", + )), + test_collection_bundle_meta_created_at: Some(String::from("2026-05-10T12:34:56.000Z")), + repo_id: None, + test_collection_id: None, + }) + } + + let mut builder = MockServerBuilder::new(); + builder.set_create_bundle_handler(create_bundle_without_collection_ids); + let state = builder.spawn_mock_server().await; + let assert = CommandBuilder::upload(temp_dir.path(), state.host.clone()) .command() .arg("--test-collection-id") @@ -332,7 +379,8 @@ async fn upload_bundle_prints_test_collection_links() { .stderr(predicate::str::contains( "/test-org/flaky-tests/collections/tc_123/t/", )) - .stderr(predicate::str::contains("?repo=trunk-io%2Fanalytics-cli")); + .stderr(predicate::str::contains("?repo=trunk-io%2Fanalytics-cli")) + .stderr(predicate::str::contains("/collections/tc_123/tests/").not()); } // NOTE: must be multi threaded to start a mock server @@ -757,6 +805,8 @@ async fn upload_bundle_without_canonical_test_collection_metadata_keeps_bundle_g key: String::from("unused"), test_collection_bundle_meta_id: None, test_collection_bundle_meta_created_at: None, + repo_id: None, + test_collection_id: None, })) }, ); @@ -1601,6 +1651,8 @@ async fn quarantines_tests_regardless_of_upload() { test_collection_bundle_meta_created_at: Some(String::from( "2026-05-10T12:34:56.000Z", )), + repo_id: Some(String::from(MOCK_REPO_ID)), + test_collection_id: Some(String::from(MOCK_TEST_COLLECTION_ID)), }) .into_response() } @@ -2340,6 +2392,8 @@ async fn do_not_quarantines_tests_when_quarantine_disabled_set() { test_collection_bundle_meta_created_at: Some(String::from( "2026-05-10T12:34:56.000Z", )), + repo_id: Some(String::from(MOCK_REPO_ID)), + test_collection_id: Some(String::from(MOCK_TEST_COLLECTION_ID)), })) } }; diff --git a/test_utils/src/mock_server.rs b/test_utils/src/mock_server.rs index ee014bb8..8f14d0a6 100644 --- a/test_utils/src/mock_server.rs +++ b/test_utils/src/mock_server.rs @@ -24,6 +24,9 @@ use proto::upload_metrics::trunk::UploadMetrics; use tempfile::tempdir; use tokio::{net::TcpListener, spawn}; +pub const MOCK_REPO_ID: &str = "7a1f0e3d-2b4c-4d5e-8f90-123456789abc"; +pub const MOCK_TEST_COLLECTION_ID: &str = "018f6d3a-6f2e-4c4a-9b1e-2f3a4b5c6d7e"; + #[derive(Debug, Clone, PartialEq)] pub enum RequestPayload { CreateBundleUpload(CreateBundleUploadRequest), @@ -180,6 +183,8 @@ pub async fn create_bundle_handler( key: String::from("unused"), test_collection_bundle_meta_id: Some(String::from("82c6a6e5-f8ea-4d93-9a26-b8ab6ff8f6bc")), test_collection_bundle_meta_created_at: Some(String::from("2026-05-10T12:34:56.000Z")), + repo_id: Some(String::from(MOCK_REPO_ID)), + test_collection_id: Some(String::from(MOCK_TEST_COLLECTION_ID)), }) }