From 9f0022e59cad677753c149e0da66f8d9c819e777 Mon Sep 17 00:00:00 2001 From: Chris Olszewski Date: Tue, 22 Oct 2024 13:31:38 -0400 Subject: [PATCH 1/5] chore(api): use insta for error message tests --- Cargo.lock | 1 + crates/turborepo-api-client/Cargo.toml | 1 + crates/turborepo-api-client/src/lib.rs | 7 ++++--- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 32634a5198221..31da40d044256 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6108,6 +6108,7 @@ dependencies = [ "chrono", "http 0.2.11", "httpmock", + "insta", "lazy_static", "port_scanner", "regex", diff --git a/crates/turborepo-api-client/Cargo.toml b/crates/turborepo-api-client/Cargo.toml index c0fff6459900b..2022a5bacc920 100644 --- a/crates/turborepo-api-client/Cargo.toml +++ b/crates/turborepo-api-client/Cargo.toml @@ -13,6 +13,7 @@ rustls-tls = ["reqwest/rustls-tls-native-roots"] anyhow = { workspace = true } http = "0.2.9" httpmock = { workspace = true } +insta = { workspace = true } port_scanner = { workspace = true } test-case = { workspace = true } turborepo-vercel-api-mock = { workspace = true } diff --git a/crates/turborepo-api-client/src/lib.rs b/crates/turborepo-api-client/src/lib.rs index f81e95c820a47..67b3d284e65c2 100644 --- a/crates/turborepo-api-client/src/lib.rs +++ b/crates/turborepo-api-client/src/lib.rs @@ -809,6 +809,7 @@ mod test { use anyhow::Result; use bytes::Bytes; + use insta::assert_snapshot; use turborepo_vercel_api_mock::start_test_server; use url::Url; @@ -886,9 +887,9 @@ mod test { .unwrap(), ); let err = APIClient::handle_403(response).await; - assert_eq!( + assert_snapshot!( err.to_string(), - "unable to parse 'this isn't valid JSON' as JSON: expected ident at line 1 column 2" + @"unable to parse 'this isn't valid JSON' as JSON: expected ident at line 1 column 2" ); } @@ -900,7 +901,7 @@ mod test { .unwrap(), ); let err = APIClient::handle_403(response).await; - assert_eq!(err.to_string(), "unknown status forbidden: Not authorized"); + assert_snapshot!(err.to_string(), @"unknown status forbidden: Not authorized"); } #[tokio::test] From bd0ed40f3bbbaa15dd53d5a9d6ead1762307ed7f Mon Sep 17 00:00:00 2001 From: Chris Olszewski Date: Tue, 22 Oct 2024 16:00:51 -0400 Subject: [PATCH 2/5] chore(cache): add http error message snapshot tests --- Cargo.lock | 2 + crates/turborepo-cache/Cargo.toml | 2 + crates/turborepo-cache/src/http.rs | 72 +++++++++++++++++++++++------- 3 files changed, 60 insertions(+), 16 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 31da40d044256..b6c6fcc21cd95 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6168,6 +6168,7 @@ dependencies = [ "camino", "futures", "hmac", + "insta", "libc", "os_str_bytes", "path-clean", @@ -6189,6 +6190,7 @@ dependencies = [ "turbopath", "turborepo-analytics", "turborepo-api-client", + "turborepo-vercel-api", "turborepo-vercel-api-mock", "zstd", ] diff --git a/crates/turborepo-cache/Cargo.toml b/crates/turborepo-cache/Cargo.toml index 08ea0eed08933..5407fb706a531 100644 --- a/crates/turborepo-cache/Cargo.toml +++ b/crates/turborepo-cache/Cargo.toml @@ -13,10 +13,12 @@ rustls-tls = ["turborepo-api-client/rustls-tls"] [dev-dependencies] anyhow = { workspace = true, features = ["backtrace"] } futures = { workspace = true } +insta = { workspace = true } libc = "0.2.146" port_scanner = { workspace = true } tempfile = { workspace = true } test-case = { workspace = true } +turborepo-vercel-api = { workspace = true } turborepo-vercel-api-mock = { workspace = true } [lints] diff --git a/crates/turborepo-cache/src/http.rs b/crates/turborepo-cache/src/http.rs index c4936526241a9..8b0649ac54314 100644 --- a/crates/turborepo-cache/src/http.rs +++ b/crates/turborepo-cache/src/http.rs @@ -105,8 +105,7 @@ impl HTTPCache { tracing::debug!("uploading {}", hash); - match self - .client + self.client .put_artifact( hash, progress, @@ -118,19 +117,9 @@ impl HTTPCache { self.api_auth.team_slug.as_deref(), ) .await - { - Ok(_) => { - tracing::debug!("uploaded {}", hash); - Ok(()) - } - Err(turborepo_api_client::Error::ReqwestError(e)) if e.is_timeout() => { - Err(CacheError::TimeoutError(hash.to_string())) - } - Err(turborepo_api_client::Error::ReqwestError(e)) if e.is_connect() => { - Err(CacheError::ConnectError) - } - Err(e) => Err(e.into()), - } + .map_err(|err| Self::convert_api_error(hash, err))?; + tracing::debug!("uploaded {}", hash); + Ok(()) } #[tracing::instrument(skip_all)] @@ -278,14 +267,27 @@ impl HTTPCache { let mut cache_reader = CacheReader::from_reader(body, true)?; cache_reader.restore(root) } + + fn convert_api_error(hash: &str, err: turborepo_api_client::Error) -> CacheError { + match err { + turborepo_api_client::Error::ReqwestError(e) if e.is_timeout() => { + CacheError::TimeoutError(hash.to_string()) + } + turborepo_api_client::Error::ReqwestError(e) if e.is_connect() => { + CacheError::ConnectError + } + e => e.into(), + } + } } #[cfg(test)] mod test { - use std::time::Duration; + use std::{backtrace::Backtrace, time::Duration}; use anyhow::Result; use futures::future::try_join_all; + use insta::assert_snapshot; use tempfile::tempdir; use turbopath::AbsoluteSystemPathBuf; use turborepo_analytics::start_analytics; @@ -381,4 +383,42 @@ mod test { Ok(()) } + + #[test] + fn test_forbidden_error() { + let err = HTTPCache::convert_api_error( + "hash", + turborepo_api_client::Error::UnknownStatus { + code: "forbidden".into(), + message: "Not authorized".into(), + backtrace: Backtrace::capture(), + }, + ); + assert_snapshot!(err.to_string(), @"failed to contact remote cache: unknown status forbidden: Not authorized"); + } + + #[test] + fn test_unknown_status() { + let err = HTTPCache::convert_api_error( + "hash", + turborepo_api_client::Error::UnknownStatus { + code: "unknown".into(), + message: "Special message".into(), + backtrace: Backtrace::capture(), + }, + ); + assert_snapshot!(err.to_string(), @"failed to contact remote cache: unknown status unknown: Special message"); + } + + #[test] + fn test_cache_disabled() { + let err = HTTPCache::convert_api_error( + "hash", + turborepo_api_client::Error::CacheDisabled { + status: turborepo_vercel_api::CachingStatus::Disabled, + message: "Cache disabled".into(), + }, + ); + assert_snapshot!(err.to_string(), @"failed to contact remote cache: Cache disabled"); + } } From 3c9a9ea8b14b72b280db731ff72f1662be330251 Mon Sep 17 00:00:00 2001 From: Chris Olszewski Date: Tue, 22 Oct 2024 16:08:52 -0400 Subject: [PATCH 3/5] chore(cache): better error message for forbidden write --- crates/turborepo-cache/src/http.rs | 5 ++++- crates/turborepo-cache/src/lib.rs | 2 ++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/crates/turborepo-cache/src/http.rs b/crates/turborepo-cache/src/http.rs index 8b0649ac54314..a9b219562cb60 100644 --- a/crates/turborepo-cache/src/http.rs +++ b/crates/turborepo-cache/src/http.rs @@ -276,6 +276,9 @@ impl HTTPCache { turborepo_api_client::Error::ReqwestError(e) if e.is_connect() => { CacheError::ConnectError } + turborepo_api_client::Error::UnknownStatus { code, .. } if code == "forbidden" => { + CacheError::ForbiddenRemoteCacheWrite + } e => e.into(), } } @@ -394,7 +397,7 @@ mod test { backtrace: Backtrace::capture(), }, ); - assert_snapshot!(err.to_string(), @"failed to contact remote cache: unknown status forbidden: Not authorized"); + assert_snapshot!(err.to_string(), @"Insufficient permissions to write to remote cache. Please verify that your role has write access for Remote Cache Artifact at https://vercel.com/docs/accounts/team-members-and-roles/access-roles/team-level-roles"); } #[test] diff --git a/crates/turborepo-cache/src/lib.rs b/crates/turborepo-cache/src/lib.rs index e839ff2608d32..5f0760f9fe796 100644 --- a/crates/turborepo-cache/src/lib.rs +++ b/crates/turborepo-cache/src/lib.rs @@ -83,6 +83,8 @@ pub enum CacheError { ConfigCacheInvalidBase, #[error("Unable to hash config cache inputs")] ConfigCacheError, + #[error("Insufficient permissions to write to remote cache. Please verify that your role has write access for Remote Cache Artifact at https://vercel.com/docs/accounts/team-members-and-roles/access-roles/team-level-roles")] + ForbiddenRemoteCacheWrite, } impl From for CacheError { From 1574b5ade6e58314dc5d7ec56032efdde66be809 Mon Sep 17 00:00:00 2001 From: Chris Olszewski Date: Tue, 22 Oct 2024 16:31:04 -0400 Subject: [PATCH 4/5] chore: include query params Co-authored-by: Thomas Knickman --- crates/turborepo-cache/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/turborepo-cache/src/lib.rs b/crates/turborepo-cache/src/lib.rs index 5f0760f9fe796..c05f4dcce5edd 100644 --- a/crates/turborepo-cache/src/lib.rs +++ b/crates/turborepo-cache/src/lib.rs @@ -83,7 +83,7 @@ pub enum CacheError { ConfigCacheInvalidBase, #[error("Unable to hash config cache inputs")] ConfigCacheError, - #[error("Insufficient permissions to write to remote cache. Please verify that your role has write access for Remote Cache Artifact at https://vercel.com/docs/accounts/team-members-and-roles/access-roles/team-level-roles")] + #[error("Insufficient permissions to write to remote cache. Please verify that your role has write access for Remote Cache Artifact at https://vercel.com/docs/accounts/team-members-and-roles/access-roles/team-level-roles?resource=Remote+Cache+Artifact")] ForbiddenRemoteCacheWrite, } From aa33aa7820aa101660c6305853c8abb779b2b99f Mon Sep 17 00:00:00 2001 From: Chris Olszewski Date: Tue, 22 Oct 2024 17:18:03 -0400 Subject: [PATCH 5/5] chore: update snapshot --- crates/turborepo-cache/src/http.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/turborepo-cache/src/http.rs b/crates/turborepo-cache/src/http.rs index a9b219562cb60..d8f89973bf568 100644 --- a/crates/turborepo-cache/src/http.rs +++ b/crates/turborepo-cache/src/http.rs @@ -397,7 +397,7 @@ mod test { backtrace: Backtrace::capture(), }, ); - assert_snapshot!(err.to_string(), @"Insufficient permissions to write to remote cache. Please verify that your role has write access for Remote Cache Artifact at https://vercel.com/docs/accounts/team-members-and-roles/access-roles/team-level-roles"); + assert_snapshot!(err.to_string(), @"Insufficient permissions to write to remote cache. Please verify that your role has write access for Remote Cache Artifact at https://vercel.com/docs/accounts/team-members-and-roles/access-roles/team-level-roles?resource=Remote+Cache+Artifact"); } #[test]