feat(upload): link to test collection pages when a collection id is passed

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<String, ParseError> {
     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,44 +27,81 @@ 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/<short_id>/tests/<repo_id>_<test_case_id> 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())
             .collect();
     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"
+            )),
+        );
+    }
 }

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<i32>,
     test_collection_short_id: Option<String>,
+    hide_test_collection_links: bool,
 ) -> anyhow::Result<QuarantineContext> {
     // 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?
     };

cli/src/context_quarantine.rs

@@ -304,6 +304,7 @@ pub async fn gather_quarantine_context(
     failed_tests_extractor: Option<FailedTestsExtractor>,
     test_run_exit_code: Option<i32>,
     variant: &String,
+    hide_test_collection_links: bool,
 ) -> anyhow::Result<QuarantineContext> {
     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(""),

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<String>,
+    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<Lines> {
             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("⤷ ")?,

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() {