refactor(upload): resolve the upload id before the quarantine step

cli/src/context.rs

@@ -9,7 +9,7 @@ use std::{
     time::{SystemTime, UNIX_EPOCH},
 };
 
-use api::{client::ApiClient, message::CreateBundleUploadResponse};
+use api::{client::ApiClient, message::CreateBundleUploadResponse, urls::TestCaseGuidScope};
 use bundle::{
     BundleMeta, BundleMetaBaseProps, BundleMetaDebugProps, BundleMetaJunitProps, BundledFile,
     FileSet, FileSetBuilder, FileSetType, INTERNAL_BIN_FILENAME, META_VERSION,
@@ -773,6 +773,7 @@ pub async fn gather_exit_code_and_quarantined_tests_context(
     default_exit_code: Option<i32>,
     test_collection_short_id: Option<String>,
     hide_test_collection_links: bool,
+    guid_scope: Option<&TestCaseGuidScope>,
 ) -> anyhow::Result<QuarantineContext> {
     // Run the quarantine step and update the exit code.
     let failed_tests_extractor = FailedTestsExtractor::new(
@@ -825,6 +826,7 @@ pub async fn gather_exit_code_and_quarantined_tests_context(
             default_exit_code,
             &meta.variant.clone().unwrap_or(String::from("")),
             hide_test_collection_links,
+            guid_scope,
         )
         .await?
     };

cli/src/context_quarantine.rs

@@ -3,7 +3,11 @@ use std::{
     io::{BufReader, Read},
 };
 
-use api::{client::ApiClient, message::QuarantineResolutionMode, urls::url_for_test_case};
+use api::{
+    client::ApiClient,
+    message::QuarantineResolutionMode,
+    urls::{TestCaseGuidScope, url_for_test_case},
+};
 use bundle::{
     FileSet, FileSetBuilder, FileSetTestRunnerReport, FileSetType, QuarantineBulkTestStatus, Test,
 };
@@ -305,6 +309,7 @@ pub async fn gather_quarantine_context(
     test_run_exit_code: Option<i32>,
     variant: &String,
     hide_test_collection_links: bool,
+    guid_scope: Option<&TestCaseGuidScope>,
 ) -> anyhow::Result<QuarantineContext> {
     let failed_tests_extractor = failed_tests_extractor.unwrap_or_else(|| {
         FailedTestsExtractor::new(
@@ -419,6 +424,7 @@ pub async fn gather_quarantine_context(
                 request,
                 api_client,
                 hide_test_collection_links,
+                guid_scope,
             )
         });
     }
@@ -430,7 +436,13 @@ pub async fn gather_quarantine_context(
             pluralize("failure", quarantined_failures.len() as isize, false),
         );
         failures.iter().for_each(|failure| {
-            log_failure(failure, request, api_client, hide_test_collection_links)
+            log_failure(
+                failure,
+                request,
+                api_client,
+                hide_test_collection_links,
+                guid_scope,
+            )
         });
     }
     let quarantined_failure_count = quarantined_failures.len();
@@ -479,19 +491,20 @@ fn log_failure(
     request: &api::message::GetQuarantineConfigRequest,
     api_client: &ApiClient,
     hide_test_collection_links: bool,
+    guid_scope: Option<&TestCaseGuidScope>,
 ) {
     let test_collection_short_id = request
         .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 guid_scope = guid_scope.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,
-        None,
+        guid_scope,
     ) {
         Ok(url) => format!("Learn more > {}", url),
         Err(_) => String::from(""),

cli/src/upload_command.rs

@@ -4,6 +4,7 @@ use std::path::PathBuf;
 use std::sync::mpsc::Sender;
 
 use api::client::{ApiClient, ApiErrorEndpoint};
+use api::message::CreateBundleUploadResponse;
 use api::urls::{TestCaseGuidScope, url_for_test_case, url_for_upload};
 use bundle::{BundleMeta, BundlerUtil, QuarantineResolutionMode, Test, unzip_tarball};
 use clap::{ArgAction, Args};
@@ -494,6 +495,33 @@ pub async fn run_upload(
         .or_else(|| test_run_result.as_ref().map(|r| r.exit_code));
     let disable_quarantining =
         upload_args.disable_quarantining || !upload_args.use_quarantining.unwrap_or(true);
+
+    // Ahead of the quarantine step so the failures it logs can be addressed by GUID. Its error
+    // is deliberately held until the upload below, where it has always surfaced -- failing here
+    // would skip quarantine resolution and change the exit code.
+    let upload_id_context = gather_upload_id_context(
+        &mut meta,
+        upload_args.test_collection_short_id.clone(),
+        &api_client,
+        upload_args.dry_run,
+    )
+    .await;
+    let guid_scope = upload_id_context.as_ref().ok().and_then(|upload| {
+        upload
+            .test_collection_id
+            .clone()
+            .zip(upload.repo_id.clone())
+            .map(|(test_collection_id, repo_id)| TestCaseGuidScope {
+                test_collection_id,
+                repo_id,
+            })
+    });
+    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"
+        );
+    }
+
     let quarantine_context = match gather_exit_code_and_quarantined_tests_context(
         &mut meta,
         disable_quarantining,
@@ -505,6 +533,7 @@ pub async fn run_upload(
             .clone()
             .filter(|id| !id.is_empty()),
         upload_args.hide_test_collection_links,
+        guid_scope.as_ref(),
     )
     .await
     {
@@ -578,8 +607,8 @@ pub async fn run_upload(
     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(),
+        &meta,
+        upload_id_context,
         &api_client,
         bep_result,
         quarantine_context.exit_code,
@@ -634,32 +663,21 @@ pub async fn run_upload(
     if upload_bundle_result.is_err() {
         tracing::error!("Failed to upload bundle");
     }
-    let (guid_scope, error_report) = match upload_bundle_result {
+    let 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(&uploaded.tarball, &bundle_file)?;
             }
-            (uploaded.guid_scope, None)
+            None
         }
-        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(),
-                ),
-            )),
-        ),
+        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()),
+        )),
     };
-    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,
@@ -680,25 +698,16 @@ struct UploadedBundle {
     tarball: PathBuf,
     // directory is removed on drop
     _temp_dir: TempDir,
-    guid_scope: Option<TestCaseGuidScope>,
 }
 
 async fn upload_bundle(
-    meta: &mut BundleMeta,
-    requested_test_collection_short_id: Option<String>,
+    meta: &BundleMeta,
+    upload_result: anyhow::Result<CreateBundleUploadResponse>,
     api_client: &ApiClient,
     bep_result: Option<BepParseResult>,
     exit_code: i32,
     dry_run: bool,
 ) -> anyhow::Result<UploadedBundle> {
-    let upload_result = gather_upload_id_context(
-        meta,
-        requested_test_collection_short_id,
-        api_client,
-        dry_run,
-    )
-    .await;
-
     let (
         bundle_temp_file,
         // directory is removed on drop
@@ -711,7 +720,6 @@ async fn upload_bundle(
         return Ok(UploadedBundle {
             tarball: bundle_temp_file,
             _temp_dir: bundle_temp_dir,
-            guid_scope: None,
         });
     }
 
@@ -732,12 +740,6 @@ async fn upload_bundle(
             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) => {

cli/tests/upload.rs

@@ -114,6 +114,31 @@ async fn upload_bundle() {
     assert_eq!(requests.len(), 4);
     let mut requests_iter = requests.into_iter();
 
+    // createBundleUpload precedes the quarantine call: the ids it returns are what let
+    // the quarantine step address each failure it logs by GUID.
+    let upload_request = assert_matches!(requests_iter.next().unwrap(), RequestPayload::CreateBundleUpload(ur) => ur);
+    assert_eq!(
+        upload_request.repo,
+        Repo {
+            host: String::from("github.com"),
+            owner: String::from("trunk-io"),
+            name: String::from("analytics-cli"),
+        }
+    );
+    assert_eq!(upload_request.org_url_slug, "test-org");
+    assert!(
+        upload_request
+            .client_version
+            .starts_with("trunk-analytics-cli cargo=")
+    );
+    assert!(upload_request.client_version.contains(" git="));
+    assert!(upload_request.client_version.contains(" rustc="));
+    assert!(upload_request.external_id.is_some());
+    assert_eq!(
+        upload_request.test_collection_short_id,
+        Some(String::from("tc_123"))
+    );
+
     let quarantine_request = requests_iter.next().unwrap();
     let mut failure_count = 0;
     assert_matches!(quarantine_request, RequestPayload::GetQuarantineConfig(req) => {
@@ -143,29 +168,6 @@ async fn upload_bundle() {
         }
     });
 
-    let upload_request = assert_matches!(requests_iter.next().unwrap(), RequestPayload::CreateBundleUpload(ur) => ur);
-    assert_eq!(
-        upload_request.repo,
-        Repo {
-            host: String::from("github.com"),
-            owner: String::from("trunk-io"),
-            name: String::from("analytics-cli"),
-        }
-    );
-    assert_eq!(upload_request.org_url_slug, "test-org");
-    assert!(
-        upload_request
-            .client_version
-            .starts_with("trunk-analytics-cli cargo=")
-    );
-    assert!(upload_request.client_version.contains(" git="));
-    assert!(upload_request.client_version.contains(" rustc="));
-    assert!(upload_request.external_id.is_some());
-    assert_eq!(
-        upload_request.test_collection_short_id,
-        Some(String::from("tc_123"))
-    );
-
     let tar_extract_directory =
         assert_matches!(requests_iter.next().unwrap(), RequestPayload::S3Upload(d) => d);
 
@@ -339,6 +341,32 @@ async fn upload_bundle_prints_test_collection_links() {
         .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_prints_guid_links_in_per_failure_logs() {
+    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;
+
+    // These lines are tracing, so they reach stdout only above info level -- the report table
+    // on stderr is what a default run shows.
+    let assert = CommandBuilder::upload(temp_dir.path(), state.host.clone())
+        .verbose(true)
+        .command()
+        .arg("--test-collection-id")
+        .arg("tc_123")
+        .assert()
+        .failure();
+
+    assert
+        .stdout(predicate::str::contains(
+            "/test-org/flaky-tests/collections/tc_123/tests/",
+        ))
+        .stdout(predicate::str::contains("/collections/tc_123/t/").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() {
@@ -2019,12 +2047,12 @@ async fn test_variant_propagation() {
 
     assert_matches!(
         requests_iter.next().unwrap(),
-        RequestPayload::GetQuarantineConfig(_)
+        RequestPayload::CreateBundleUpload(_)
     );
 
     assert_matches!(
         requests_iter.next().unwrap(),
-        RequestPayload::CreateBundleUpload(_)
+        RequestPayload::GetQuarantineConfig(_)
     );
 
     let tar_extract_directory =