diff --git a/cli/src/context.rs b/cli/src/context.rs index 4363f3d2..e7538c00 100644 --- a/cli/src/context.rs +++ b/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, test_collection_short_id: Option, hide_test_collection_links: bool, + guid_scope: Option<&TestCaseGuidScope>, ) -> anyhow::Result { // 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? }; diff --git a/cli/src/context_quarantine.rs b/cli/src/context_quarantine.rs index 6b4fd7a7..366647a8 100644 --- a/cli/src/context_quarantine.rs +++ b/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, variant: &String, hide_test_collection_links: bool, + guid_scope: Option<&TestCaseGuidScope>, ) -> anyhow::Result { 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(""), diff --git a/cli/src/upload_command.rs b/cli/src/upload_command.rs index 05e95448..76bd0a44 100644 --- a/cli/src/upload_command.rs +++ b/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, } async fn upload_bundle( - meta: &mut BundleMeta, - requested_test_collection_short_id: Option, + meta: &BundleMeta, + upload_result: anyhow::Result, api_client: &ApiClient, bep_result: Option, exit_code: i32, dry_run: bool, ) -> anyhow::Result { - 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) => { diff --git a/cli/tests/upload.rs b/cli/tests/upload.rs index 3e3d4f40..a0db41d5 100644 --- a/cli/tests/upload.rs +++ b/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 =