From f4532eee40bc6b7d19634c70a43a7bd78138a42d Mon Sep 17 00:00:00 2001 From: Justin Geibel Date: Fri, 15 Mar 2019 18:30:22 -0400 Subject: [PATCH 1/5] Reduce the number of git clones in the test framework The test framework spends a lot of I/O and allocations initializing and cloning git repositories, even though most tests do not enqueue a background job to modify the index. A `TestApp::full()` method is added which initializes the `TestApp` with an index, background job runner, and playback proxy. It is also now possible to enqueue multiple crate publishes before running a batch of background jobs. A `Drop` impl on `TestAppInner` ensures that all pending migrations are run and that no jobs have been queued unless `TestApp::full()` was used. Unlike the `enqueue_publish` helper the `yank` and `unyank` methods automatically run migrations out of convenience since otherwise the database is not updated. This can be changed in the future if a test cares about controlling exactly when the jobs are run. Finally, the initial commit for each remaining upstream index is now made directly against the bare repo, rather than doing a clone and push. --- src/middleware.rs | 6 - src/middleware/run_pending_background_jobs.rs | 55 ----- src/tests/all.rs | 2 - src/tests/git.rs | 39 +--- src/tests/krate.rs | 201 ++++++++---------- src/tests/owners.rs | 2 +- src/tests/read_only_mode.rs | 8 + src/tests/team.rs | 2 +- src/tests/util.rs | 133 +++++++++++- src/tests/version.rs | 3 +- 10 files changed, 231 insertions(+), 220 deletions(-) delete mode 100644 src/middleware/run_pending_background_jobs.rs diff --git a/src/middleware.rs b/src/middleware.rs index e34cb38dc0f..efa5e9f251b 100644 --- a/src/middleware.rs +++ b/src/middleware.rs @@ -10,7 +10,6 @@ pub use self::debug::*; pub use self::ember_index_rewrite::EmberIndexRewrite; pub use self::head::Head; use self::log_connection_pool_status::LogConnectionPoolStatus; -use self::run_pending_background_jobs::RunPendingBackgroundJobs; pub use self::security_headers::SecurityHeaders; pub use self::static_or_continue::StaticOrContinue; @@ -24,7 +23,6 @@ mod head; mod log_connection_pool_status; mod log_request; mod require_user_agent; -mod run_pending_background_jobs; mod security_headers; mod static_or_continue; @@ -102,9 +100,5 @@ pub fn build_middleware(app: Arc, endpoints: R404) -> MiddlewareBuilder { m.around(log_request::LogRequests::default()); } - if env == Env::Test { - m.add(RunPendingBackgroundJobs); - } - m } diff --git a/src/middleware/run_pending_background_jobs.rs b/src/middleware/run_pending_background_jobs.rs deleted file mode 100644 index dbea033b7d6..00000000000 --- a/src/middleware/run_pending_background_jobs.rs +++ /dev/null @@ -1,55 +0,0 @@ -use std::time::Duration; -use swirl::Runner; - -use super::app::RequestApp; -use super::prelude::*; -use crate::background_jobs::*; -use crate::git::Repository; - -pub struct RunPendingBackgroundJobs; - -impl Middleware for RunPendingBackgroundJobs { - fn after( - &self, - req: &mut dyn Request, - res: Result>, - ) -> Result> { - if response_is_error(&res) { - return res; - } - - let app = req.app(); - - let connection_pool = app.diesel_database.clone(); - let repo = Repository::open(&app.config.index_location).expect("Could not clone index"); - let environment = Environment::new(repo, None, app.diesel_database.clone()); - - let runner = Runner::builder(connection_pool, environment) - // We only have 1 connection in tests, so trying to run more than - // 1 job concurrently will just block - .thread_count(1) - .job_start_timeout(Duration::from_secs(1)) - .build(); - - // FIXME: https://github.com/sgrif/swirl/issues/8 - if let Err(e) = runner.run_all_pending_jobs() { - if e.to_string().ends_with("read-only transaction") { - return res; - } else { - panic!("Could not run jobs: {}", e); - } - } - - runner - .assert_no_failed_jobs() - .expect("Could not determine if jobs failed"); - res - } -} - -fn response_is_error(res: &Result>) -> bool { - match res { - Ok(res) => res.status.0 >= 400, - Err(_) => true, - } -} diff --git a/src/tests/all.rs b/src/tests/all.rs index 24e5344f151..2d449729953 100644 --- a/src/tests/all.rs +++ b/src/tests/all.rs @@ -119,8 +119,6 @@ fn app() -> ( } fn simple_app(proxy: Option) -> (Arc, conduit_middleware::MiddlewareBuilder) { - git::init(); - let uploader = Uploader::S3 { bucket: s3::Bucket::new( String::from("alexcrichton-test"), diff --git a/src/tests/git.rs b/src/tests/git.rs index cf10e60b73e..7d1e87310e6 100644 --- a/src/tests/git.rs +++ b/src/tests/git.rs @@ -4,12 +4,11 @@ use std::path::PathBuf; use std::sync::Once; use std::thread; -use url::Url; - fn root() -> PathBuf { env::current_dir() .unwrap() .join("tmp") + .join("tests") .join(thread::current().name().unwrap()) } @@ -29,38 +28,14 @@ pub fn init() { fs::create_dir_all(root().parent().unwrap()).unwrap(); }); - // Prepare a bare remote repo - { - let bare = git2::Repository::init_bare(&bare()).unwrap(); - let mut config = bare.config().unwrap(); - config.set_str("user.name", "name").unwrap(); - config.set_str("user.email", "email").unwrap(); - } - - // Initialize a fresh checkout - let checkout = git2::Repository::init(&checkout()).unwrap(); - let url = Url::from_file_path(&*bare()).ok().unwrap().to_string(); - - // Setup the `origin` remote - checkout.remote_set_url("origin", &url).unwrap(); - checkout.remote_set_pushurl("origin", Some(&url)).unwrap(); - checkout - .remote_add_push("origin", "refs/heads/master") - .unwrap(); - - // Create an empty initial commit - let mut config = checkout.config().unwrap(); + let bare = git2::Repository::init_bare(&bare()).unwrap(); + let mut config = bare.config().unwrap(); config.set_str("user.name", "name").unwrap(); config.set_str("user.email", "email").unwrap(); - let mut index = checkout.index().unwrap(); + let mut index = bare.index().unwrap(); let id = index.write_tree().unwrap(); - let tree = checkout.find_tree(id).unwrap(); - let sig = checkout.signature().unwrap(); - checkout - .commit(Some("HEAD"), &sig, &sig, "Initial Commit", &tree, &[]) + let tree = bare.find_tree(id).unwrap(); + let sig = bare.signature().unwrap(); + bare.commit(Some("HEAD"), &sig, &sig, "Initial Commit", &tree, &[]) .unwrap(); - - // Push the commit to the remote repo - let mut origin = checkout.find_remote("origin").unwrap(); - origin.push(&["refs/heads/master"], None).unwrap(); } diff --git a/src/tests/krate.rs b/src/tests/krate.rs index a14621fe554..41628503099 100644 --- a/src/tests/krate.rs +++ b/src/tests/krate.rs @@ -4,7 +4,6 @@ use crate::{ RequestHelper, TestApp, }; use cargo_registry::{ - git, models::{krate::MAX_NAME_LENGTH, Category, Crate}, schema::{api_tokens, crates, emails, metadata, versions, versions_published_by}, views::{ @@ -14,14 +13,12 @@ use cargo_registry::{ }; use std::{ collections::HashMap, - fs::{self, File}, io::{self, prelude::*}, }; use chrono::Utc; use diesel::{dsl::*, prelude::*, update}; use flate2::{write::GzEncoder, Compression}; -use tempdir::TempDir; #[derive(Deserialize)] struct VersionsList { @@ -62,16 +59,20 @@ impl crate::util::MockAnonymousUser { } impl crate::util::MockTokenUser { - /// Yank the specified version of the specified crate. + /// Yank the specified version of the specified crate and run all pending background jobs fn yank(&self, krate_name: &str, version: &str) -> crate::util::Response { let url = format!("/api/v1/crates/{}/{}/yank", krate_name, version); - self.delete(&url) + let response = self.delete(&url); + self.app().run_pending_background_jobs(); + response } - /// Unyank the specified version of the specified crate. + /// Unyank the specified version of the specified crate and run all pending background jobs fn unyank(&self, krate_name: &str, version: &str) -> crate::util::Response { let url = format!("/api/v1/crates/{}/{}/unyank", krate_name, version); - self.put(&url, &[]) + let response = self.put(&url, &[]); + self.app().run_pending_background_jobs(); + response } } @@ -572,7 +573,8 @@ fn versions() { fn uploading_new_version_touches_crate() { use diesel::dsl::*; - let (app, _, user) = TestApp::with_proxy().with_user(); + let (app, _, user) = TestApp::full().with_user(); + let crate_to_publish = PublishBuilder::new("foo_versions_updated_at").version("1.0.0"); user.publish(crate_to_publish).good(); @@ -631,8 +633,9 @@ fn new_wrong_token() { #[test] fn invalid_names() { - fn bad_name(name: &str, error_message: &str) { - let (_, _, _, token) = TestApp::init().with_token(); + let (_, _, _, token) = TestApp::init().with_token(); + + let bad_name = |name: &str, error_message: &str| { let crate_to_publish = PublishBuilder::new(name).version("1.0.0"); let json = token.publish(crate_to_publish).bad_with_status(200); @@ -641,7 +644,7 @@ fn invalid_names() { "{:?}", json.errors ); - } + }; let error_message = "expected a valid crate name"; bad_name("", error_message); @@ -660,7 +663,8 @@ fn invalid_names() { #[test] fn new_krate() { - let (_, _, user) = TestApp::with_proxy().with_user(); + let (_, _, user) = TestApp::full().with_user(); + let crate_to_publish = PublishBuilder::new("foo_new").version("1.0.0"); let json: GoodCrate = user.publish(crate_to_publish).good(); @@ -670,7 +674,7 @@ fn new_krate() { #[test] fn new_krate_with_token() { - let (_, _, _, token) = TestApp::with_proxy().with_token(); + let (_, _, _, token) = TestApp::full().with_token(); let crate_to_publish = PublishBuilder::new("foo_new").version("1.0.0"); let json: GoodCrate = token.publish(crate_to_publish).good(); @@ -681,7 +685,7 @@ fn new_krate_with_token() { #[test] fn new_krate_weird_version() { - let (_, _, _, token) = TestApp::with_proxy().with_token(); + let (_, _, _, token) = TestApp::full().with_token(); let crate_to_publish = PublishBuilder::new("foo_weird").version("0.0.0-pre"); let json: GoodCrate = token.publish(crate_to_publish).good(); @@ -692,7 +696,7 @@ fn new_krate_weird_version() { #[test] fn new_with_renamed_dependency() { - let (app, _, user, token) = TestApp::with_proxy().with_token(); + let (app, _, user, token) = TestApp::full().with_token(); app.db(|conn| { // Insert a crate directly into the database so that new-krate can depend on it @@ -705,26 +709,20 @@ fn new_with_renamed_dependency() { .version("1.0.0") .dependency(dependency); token.publish(crate_to_publish).good(); + app.run_pending_background_jobs(); - let remote_contents = clone_remote_repo(); - let path = remote_contents.path().join("ne/w-/new-krate"); - assert!(path.exists()); - let mut contents = String::new(); - File::open(&path) - .unwrap() - .read_to_string(&mut contents) - .unwrap(); - let p: git::Crate = serde_json::from_str(&contents).unwrap(); - assert_eq!(p.name, "new-krate"); - assert_eq!(p.vers, "1.0.0"); - assert_eq!(p.deps.len(), 1); - assert_eq!(p.deps[0].name, "my-name"); - assert_eq!(p.deps[0].package.as_ref().unwrap(), "package-name"); + let crates = app.crates_from_index_head("ne/w-/new-krate"); + assert_eq!(crates.len(), 1); + assert_eq!(crates[0].name, "new-krate"); + assert_eq!(crates[0].vers, "1.0.0"); + assert_eq!(crates[0].deps.len(), 1); + assert_eq!(crates[0].deps[0].name, "my-name"); + assert_eq!(crates[0].deps[0].package.as_ref().unwrap(), "package-name"); } #[test] fn new_krate_with_dependency() { - let (app, _, user, token) = TestApp::with_proxy().with_token(); + let (app, _, user, token) = TestApp::full().with_token(); app.db(|conn| { // Insert a crate directly into the database so that new_dep can depend on it @@ -761,7 +759,7 @@ fn reject_new_krate_with_non_exact_dependency() { #[test] fn new_crate_allow_empty_alternative_registry_dependency() { - let (app, _, user, token) = TestApp::with_proxy().with_token(); + let (app, _, user, token) = TestApp::full().with_token(); app.db(|conn| { CrateBuilder::new("foo-dep", user.as_model().id).expect_build(conn); @@ -815,7 +813,7 @@ fn new_krate_with_wildcard_dependency() { #[test] fn new_krate_twice() { - let (app, _, user, token) = TestApp::with_proxy().with_token(); + let (app, _, user, token) = TestApp::full().with_token(); app.db(|conn| { // Insert a crate directly into the database and then we'll try to publish another version @@ -868,6 +866,7 @@ fn valid_feature_names() { #[test] fn new_krate_too_big() { let (_, _, user) = TestApp::init().with_user(); + let files = [("foo_big-1.0.0/big", &[b'a'; 2000] as &[_])]; let builder = PublishBuilder::new("foo_big").files(&files); @@ -883,7 +882,7 @@ fn new_krate_too_big() { #[test] fn new_krate_too_big_but_whitelisted() { - let (app, _, user, token) = TestApp::with_proxy().with_token(); + let (app, _, user, token) = TestApp::full().with_token(); app.db(|conn| { CrateBuilder::new("foo_whitelist", user.as_model().id) @@ -1019,66 +1018,56 @@ fn new_crate_similar_name_underscore() { #[test] fn new_krate_git_upload() { - let (_, _, _, token) = TestApp::with_proxy().with_token(); + let (app, _, _, token) = TestApp::full().with_token(); + let crate_to_publish = PublishBuilder::new("fgt"); token.publish(crate_to_publish).good(); + app.run_pending_background_jobs(); - let remote_contents = clone_remote_repo(); - let path = remote_contents.path().join("3/f/fgt"); - assert!(path.exists()); - let mut contents = String::new(); - File::open(&path) - .unwrap() - .read_to_string(&mut contents) - .unwrap(); - let p: git::Crate = serde_json::from_str(&contents).unwrap(); - assert_eq!(p.name, "fgt"); - assert_eq!(p.vers, "1.0.0"); - assert!(p.deps.is_empty()); + let crates = app.crates_from_index_head("3/f/fgt"); + assert!(crates.len() == 1); + assert_eq!(crates[0].name, "fgt"); + assert_eq!(crates[0].vers, "1.0.0"); + assert!(crates[0].deps.is_empty()); assert_eq!( - p.cksum, + crates[0].cksum, "acb5604b126ac894c1eb11c4575bf2072fea61232a888e453770c79d7ed56419" ); } #[test] fn new_krate_git_upload_appends() { - let (_, _, _, token) = TestApp::with_proxy().with_token(); + let (app, _, _, token) = TestApp::full().with_token(); let crate_to_publish = PublishBuilder::new("FPP").version("0.0.1"); token.publish(crate_to_publish).good(); let crate_to_publish = PublishBuilder::new("FPP").version("1.0.0"); token.publish(crate_to_publish).good(); + app.run_pending_background_jobs(); - let remote_contents = clone_remote_repo(); - let path = remote_contents.path().join("3/f/fpp"); - let contents = fs::read_to_string(&path).unwrap(); - let mut lines = contents.lines(); - let p1: git::Crate = serde_json::from_str(lines.next().unwrap().trim()).unwrap(); - let p2: git::Crate = serde_json::from_str(lines.next().unwrap().trim()).unwrap(); - assert!(lines.next().is_none()); - assert_eq!(p1.name, "FPP"); - assert_eq!(p1.vers, "0.0.1"); - assert!(p1.deps.is_empty()); - assert_eq!(p2.name, "FPP"); - assert_eq!(p2.vers, "1.0.0"); - assert!(p2.deps.is_empty()); + let crates = app.crates_from_index_head("3/f/fpp"); + assert!(crates.len() == 2); + assert_eq!(crates[0].name, "FPP"); + assert_eq!(crates[0].vers, "0.0.1"); + assert!(crates[0].deps.is_empty()); + assert_eq!(crates[1].name, "FPP"); + assert_eq!(crates[1].vers, "1.0.0"); + assert!(crates[1].deps.is_empty()); } #[test] fn new_krate_git_upload_with_conflicts() { - { - crate::git::init(); - let repo = git2::Repository::open(&crate::git::bare()).unwrap(); - let target = repo.head().unwrap().target().unwrap(); - let sig = repo.signature().unwrap(); - let parent = repo.find_commit(target).unwrap(); - let tree = repo.find_tree(parent.tree_id()).unwrap(); - repo.commit(Some("HEAD"), &sig, &sig, "empty commit", &tree, &[&parent]) - .unwrap(); - } + let (app, _, _, token) = TestApp::full().with_token(); + + let index = app.upstream_repository(); + let target = index.head().unwrap().target().unwrap(); + let sig = index.signature().unwrap(); + let parent = index.find_commit(target).unwrap(); + let tree = index.find_tree(parent.tree_id()).unwrap(); + index + .commit(Some("HEAD"), &sig, &sig, "empty commit", &tree, &[&parent]) + .unwrap(); - let (_, _, _, token) = TestApp::with_proxy().with_token(); let crate_to_publish = PublishBuilder::new("foo_conflicts"); token.publish(crate_to_publish).good(); } @@ -1104,7 +1093,7 @@ fn new_krate_dependency_missing() { #[test] fn new_krate_with_readme() { - let (_, _, _, token) = TestApp::with_proxy().with_token(); + let (_, _, _, token) = TestApp::full().with_token(); let crate_to_publish = PublishBuilder::new("foo_readme").readme(""); let json = token.publish(crate_to_publish).good(); @@ -1160,7 +1149,7 @@ fn new_krate_with_unverified_email_fails() { #[test] fn new_krate_records_verified_email() { - let (app, _, _, token) = TestApp::with_proxy().with_token(); + let (app, _, _, token) = TestApp::full().with_token(); let crate_to_publish = PublishBuilder::new("foo_verified_email"); @@ -1392,20 +1381,16 @@ fn following() { #[test] fn yank() { - let (_, anon, _, token) = TestApp::with_proxy().with_token(); + let (app, anon, _, token) = TestApp::full().with_token(); // Upload a new crate, putting it in the git index let crate_to_publish = PublishBuilder::new("fyk"); token.publish(crate_to_publish).good(); + app.run_pending_background_jobs(); - let remote_contents = clone_remote_repo(); - let path = remote_contents.path().join("3/f/fyk"); - let mut contents = String::new(); - File::open(&path) - .unwrap() - .read_to_string(&mut contents) - .unwrap(); - assert!(contents.contains("\"yanked\":false")); + let crates = app.crates_from_index_head("3/f/fyk"); + assert!(crates.len() == 1); + assert!(!crates[0].yanked.unwrap()); // make sure it's not yanked let json = anon.show_version("fyk", "1.0.0"); @@ -1414,14 +1399,9 @@ fn yank() { // yank it token.yank("fyk", "1.0.0").good(); - let remote_contents = clone_remote_repo(); - let path = remote_contents.path().join("3/f/fyk"); - let mut contents = String::new(); - File::open(&path) - .unwrap() - .read_to_string(&mut contents) - .unwrap(); - assert!(contents.contains("\"yanked\":true")); + let crates = app.crates_from_index_head("3/f/fyk"); + assert!(crates.len() == 1); + assert!(crates[0].yanked.unwrap()); let json = anon.show_version("fyk", "1.0.0"); assert!(json.version.yanked); @@ -1429,14 +1409,9 @@ fn yank() { // un-yank it token.unyank("fyk", "1.0.0").good(); - let remote_contents = clone_remote_repo(); - let path = remote_contents.path().join("3/f/fyk"); - let mut contents = String::new(); - File::open(&path) - .unwrap() - .read_to_string(&mut contents) - .unwrap(); - assert!(contents.contains("\"yanked\":false")); + let crates = app.crates_from_index_head("3/f/fyk"); + assert!(crates.len() == 1); + assert!(!crates[0].yanked.unwrap()); let json = anon.show_version("fyk", "1.0.0"); assert!(!json.version.yanked); @@ -1444,7 +1419,8 @@ fn yank() { #[test] fn yank_not_owner() { - let (app, _, _, token) = TestApp::init().with_token(); + let (app, _, _, token) = TestApp::full().with_token(); + let another_user = app.db_new_user("bar"); let another_user = another_user.as_model(); app.db(|conn| { @@ -1461,7 +1437,7 @@ fn yank_not_owner() { #[test] #[allow(clippy::cyclomatic_complexity)] fn yank_max_version() { - let (_, anon, _, token) = TestApp::with_proxy().with_token(); + let (_, anon, _, token) = TestApp::full().with_token(); // Upload a new crate let crate_to_publish = PublishBuilder::new("fyk_max"); @@ -1515,7 +1491,7 @@ fn yank_max_version() { #[test] fn publish_after_yank_max_version() { - let (_, anon, _, token) = TestApp::with_proxy().with_token(); + let (_, anon, _, token) = TestApp::full().with_token(); // Upload a new crate let crate_to_publish = PublishBuilder::new("fyk_max"); @@ -1545,7 +1521,7 @@ fn publish_after_yank_max_version() { #[test] fn publish_after_removing_documentation() { - let (app, anon, user, token) = TestApp::with_proxy().with_token(); + let (app, anon, user, token) = TestApp::full().with_token(); let user = user.as_model(); // 1. Start with a crate with no documentation @@ -1597,7 +1573,7 @@ fn bad_keywords() { #[test] fn good_categories() { - let (app, _, _, token) = TestApp::with_proxy().with_token(); + let (app, _, _, token) = TestApp::full().with_token(); app.db(|conn| { new_category("Category 1", "cat1", "Category 1 crates") @@ -1615,7 +1591,7 @@ fn good_categories() { #[test] fn ignored_categories() { - let (_, _, _, token) = TestApp::with_proxy().with_token(); + let (_, _, _, token) = TestApp::full().with_token(); let crate_to_publish = PublishBuilder::new("foo_ignored_cat").category("bar"); let json = token.publish(crate_to_publish).good(); @@ -1627,7 +1603,7 @@ fn ignored_categories() { #[test] fn good_badges() { - let (_, anon, _, token) = TestApp::with_proxy().with_token(); + let (_, anon, _, token) = TestApp::full().with_token(); let mut badges = HashMap::new(); let mut badge_attributes = HashMap::new(); @@ -1655,7 +1631,7 @@ fn good_badges() { #[test] fn ignored_badges() { - let (_, anon, _, token) = TestApp::with_proxy().with_token(); + let (_, anon, _, token) = TestApp::full().with_token(); let mut badges = HashMap::new(); @@ -2126,14 +2102,3 @@ fn new_krate_tarball_with_hard_links() { json.errors ); } - -/// We want to observe the contents of our push, but we can't do that in a -/// bare repo so we need to clone it to some random directory. -fn clone_remote_repo() -> TempDir { - use url::Url; - - let tempdir = TempDir::new("tests").unwrap(); - let url = Url::from_file_path(crate::git::bare()).unwrap(); - git2::Repository::clone(url.as_str(), tempdir.path()).unwrap(); - tempdir -} diff --git a/src/tests/owners.rs b/src/tests/owners.rs index fc310bac654..2c8f47bddc4 100644 --- a/src/tests/owners.rs +++ b/src/tests/owners.rs @@ -59,7 +59,7 @@ impl crate::util::MockCookieUser { #[test] fn new_crate_owner() { - let (app, _, _, token) = TestApp::with_proxy().with_token(); + let (app, _, _, token) = TestApp::full().with_token(); // Create a crate under one user let crate_to_publish = PublishBuilder::new("foo_owner").version("1.0.0"); diff --git a/src/tests/read_only_mode.rs b/src/tests/read_only_mode.rs index 6f742c6f1b6..de034101c4e 100644 --- a/src/tests/read_only_mode.rs +++ b/src/tests/read_only_mode.rs @@ -21,6 +21,13 @@ fn cannot_hit_endpoint_which_writes_db_in_read_only_mode() { token .delete::<()>("/api/v1/crates/foo_yank_read_only/1.0.0/yank") .assert_status(503); + + // Restore the transaction so `TestApp::drop` can still access the transaction + app.db(|conn| { + diesel::sql_query("ROLLBACK TO test_post_readonly") + .execute(conn) + .unwrap(); + }); } #[test] @@ -51,5 +58,6 @@ fn can_download_crate_in_read_only_mode() { fn set_read_only(conn: &PgConnection) -> QueryResult<()> { diesel::sql_query("SET TRANSACTION READ ONLY").execute(conn)?; + diesel::sql_query("SAVEPOINT test_post_readonly").execute(conn)?; Ok(()) } diff --git a/src/tests/team.rs b/src/tests/team.rs index 8abc642a1a9..7b946e820ea 100644 --- a/src/tests/team.rs +++ b/src/tests/team.rs @@ -279,7 +279,7 @@ fn publish_not_owned() { // Test trying to publish a krate we do own (but only because of teams) #[test] fn publish_owned() { - let (app, _) = TestApp::with_proxy().empty(); + let (app, _) = TestApp::full().empty(); let user_on_both_teams = app.db_new_user(mock_user_on_both_teams().gh_login); let token_on_both_teams = user_on_both_teams.db_new_token("arbitrary token name"); diff --git a/src/tests/util.rs b/src/tests/util.rs index 623b8474e77..25f7934355e 100644 --- a/src/tests/util.rs +++ b/src/tests/util.rs @@ -1,6 +1,6 @@ //! This module provides utility types and traits for managing a test session //! -//! Tests start by using one of the `TestApp` constructors, `init` or `with_proxy`. This returns a +//! Tests start by using one of the `TestApp` constructors: `init`, `with_proxy`, or `full`. This returns a //! `TestAppBuilder` which provides convience methods for creating up to one user, optionally with //! a token. The builder methods all return at least an initialized `TestApp` and a //! `MockAnonymousUser`. The `MockAnonymousUser` can be used to issue requests in an @@ -24,21 +24,67 @@ use crate::{ VersionResponse, }; use cargo_registry::{ + background_jobs::Environment, + db::DieselPool, middleware::current_user::AuthenticationSource, models::{ApiToken, User}, App, }; use diesel::PgConnection; -use std::{rc::Rc, sync::Arc}; +use std::{rc::Rc, sync::Arc, time::Duration}; +use swirl::Runner; use conduit::{Handler, Method, Request}; use conduit_test::MockRequest; +use cargo_registry::git::Repository as WorkerRepository; +use git2::Repository as UpstreamRepository; + struct TestAppInner { app: Arc, // The bomb (if created) needs to be held in scope until the end of the test. _bomb: Option, middle: conduit_middleware::MiddlewareBuilder, + index: Option, + runner: Option>, +} + +use swirl::schema::background_jobs; +// FIXME: This is copied from swirl::storage, because it is private +#[derive(Queryable, Identifiable, Debug, Clone)] +struct BackgroundJob { + pub id: i64, + pub job_type: String, + pub data: serde_json::Value, +} + +impl Drop for TestAppInner { + fn drop(&mut self) { + use diesel::prelude::*; + use swirl::schema::background_jobs::dsl::*; + + // Avoid a double-panic if the test is already failing + if std::thread::panicking() { + return; + } + + // Lazily run any remaining jobs + if let Some(runner) = &self.runner { + runner.run_all_pending_jobs().expect("Could not run jobs"); + runner.assert_no_failed_jobs().expect("Failed jobs remain"); + } + + // Manually verify that all jobs have completed successfully + // This will catch any tests that enqueued a job but forgot to initialize the runner + let conn = self.app.diesel_database.get().unwrap(); + let job_count: i64 = background_jobs.count().get_result(&*conn).unwrap(); + assert_eq!( + 0, job_count, + "Unprocessed or failed jobs remain in the queue" + ); + + // TODO: If a runner was started, obtain the clone from it and ensure its HEAD matches the upstream index HEAD + } } /// A representation of the app and its database transaction @@ -52,17 +98,53 @@ impl TestApp { app, _bomb: None, middle, + index: None, + runner: None, }); TestAppBuilder(TestApp(inner)) } - /// Initialize a full application that can record and playback outgoing HTTP requests + /// Initialize the app and a proxy that can record and playback outgoing HTTP requests pub fn with_proxy() -> TestAppBuilder { let (bomb, app, middle) = app(); let inner = Rc::new(TestAppInner { app, _bomb: Some(bomb), middle, + index: None, + runner: None, + }); + TestAppBuilder(TestApp(inner)) + } + + /// Initialize a full application, with a proxy, index, and background worker + pub fn full() -> TestAppBuilder { + use crate::git; + + let (bomb, app, middle) = app(); + git::init(); + + let thread_local_path = git::bare(); + let index = UpstreamRepository::open_bare(thread_local_path).unwrap(); + + let index_clone = + WorkerRepository::open(&app.config.index_location).expect("Could not clone index"); + let connection_pool = app.diesel_database.clone(); + let environment = Environment::new(index_clone, None, connection_pool.clone()); + + let runner = Runner::builder(connection_pool, environment) + // We only have 1 connection in tests, so trying to run more than + // 1 job concurrently will just block + .thread_count(1) + .job_start_timeout(Duration::from_secs(1)) + .build(); + + let inner = Rc::new(TestAppInner { + app, + _bomb: Some(bomb), + middle, + index: Some(index), + runner: Some(runner), }); TestAppBuilder(TestApp(inner)) } @@ -105,6 +187,43 @@ impl TestApp { } } + /// Obtain a reference to the upstream repository ("the index") + pub fn upstream_repository(&self) -> &UpstreamRepository { + self.0.index.as_ref().unwrap() + } + + /// Obtain a list of crates from the index HEAD + pub fn crates_from_index_head(&self, path: &str) -> Vec { + let path = std::path::Path::new(path); + let index = self.upstream_repository(); + let tree = index.head().unwrap().peel_to_tree().unwrap(); + let blob = tree + .get_path(path) + .unwrap() + .to_object(&index) + .unwrap() + .peel_to_blob() + .unwrap(); + let content = blob.content(); + + // The index format consists of one JSON object per line + // It is not a JSON array + let lines = std::str::from_utf8(content).unwrap().lines(); + lines + .map(|line| serde_json::from_str(line).unwrap()) + .collect() + } + + pub fn run_pending_background_jobs(&self) { + let runner = &self.0.runner; + let runner = runner.as_ref().expect("Index has not been initialized"); + + runner.run_all_pending_jobs().expect("Could not run jobs"); + runner + .assert_no_failed_jobs() + .expect("Could not determine if jobs failed"); + } + /// Obtain a reference to the inner `App` value pub fn as_inner(&self) -> &App { &*self.0.app @@ -203,7 +322,13 @@ pub trait RequestHelper { self.search(&format!("user_id={}", id)) } - /// Publish a crate + /// Enqueue a crate for publishing + /// + /// The publish endpoint will enqueue a background job to update the index. A test must run + /// any pending background jobs if it intends to observe changes to the index. + /// + /// Any pending jobs are run when the `TestApp` is dropped to ensure that the test fails unless + /// all background tasks complete successfully. fn publish(&self, publish_builder: PublishBuilder) -> Response { let krate_name = publish_builder.krate_name.clone(); let response = self.put("/api/v1/crates/new", &publish_builder.body()); diff --git a/src/tests/version.rs b/src/tests/version.rs index fc5c6bc3ec5..acf7cd08748 100644 --- a/src/tests/version.rs +++ b/src/tests/version.rs @@ -137,7 +137,8 @@ fn record_rerendered_readme_time() { #[test] fn version_size() { - let (_, _, user) = TestApp::with_proxy().with_user(); + let (_, _, user) = TestApp::full().with_user(); + let crate_to_publish = PublishBuilder::new("foo_version_size").version("1.0.0"); user.publish(crate_to_publish).good(); From 1878e986f76d7534b9519dded66df852b80b9af1 Mon Sep 17 00:00:00 2001 From: Justin Geibel Date: Thu, 21 Mar 2019 00:35:43 -0400 Subject: [PATCH 2/5] Rename publish to enqueue_publish --- src/tests/krate.rs | 104 ++++++++++++++++++++++--------------------- src/tests/owners.rs | 4 +- src/tests/team.rs | 6 +-- src/tests/util.rs | 2 +- src/tests/version.rs | 4 +- 5 files changed, 61 insertions(+), 59 deletions(-) diff --git a/src/tests/krate.rs b/src/tests/krate.rs index 41628503099..b52b2e56916 100644 --- a/src/tests/krate.rs +++ b/src/tests/krate.rs @@ -576,7 +576,7 @@ fn uploading_new_version_touches_crate() { let (app, _, user) = TestApp::full().with_user(); let crate_to_publish = PublishBuilder::new("foo_versions_updated_at").version("1.0.0"); - user.publish(crate_to_publish).good(); + user.enqueue_publish(crate_to_publish).good(); app.db(|conn| { diesel::update(crates::table) @@ -589,7 +589,7 @@ fn uploading_new_version_touches_crate() { let updated_at_before = json.krate.updated_at; let crate_to_publish = PublishBuilder::new("foo_versions_updated_at").version("2.0.0"); - user.publish(crate_to_publish).good(); + user.enqueue_publish(crate_to_publish).good(); let json: CrateResponse = user.show_crate("foo_versions_updated_at"); let updated_at_after = json.krate.updated_at; @@ -603,7 +603,7 @@ fn new_wrong_token() { // Try to publish without a token let crate_to_publish = PublishBuilder::new("foo"); - let json = anon.publish(crate_to_publish).bad_with_status(403); + let json = anon.enqueue_publish(crate_to_publish).bad_with_status(403); assert!( json.errors[0] .detail @@ -621,7 +621,7 @@ fn new_wrong_token() { }); let crate_to_publish = PublishBuilder::new("foo"); - let json = token.publish(crate_to_publish).bad_with_status(403); + let json = token.enqueue_publish(crate_to_publish).bad_with_status(403); assert!( json.errors[0] .detail @@ -637,7 +637,7 @@ fn invalid_names() { let bad_name = |name: &str, error_message: &str| { let crate_to_publish = PublishBuilder::new(name).version("1.0.0"); - let json = token.publish(crate_to_publish).bad_with_status(200); + let json = token.enqueue_publish(crate_to_publish).bad_with_status(200); assert!( json.errors[0].detail.contains(error_message,), @@ -666,7 +666,7 @@ fn new_krate() { let (_, _, user) = TestApp::full().with_user(); let crate_to_publish = PublishBuilder::new("foo_new").version("1.0.0"); - let json: GoodCrate = user.publish(crate_to_publish).good(); + let json: GoodCrate = user.enqueue_publish(crate_to_publish).good(); assert_eq!(json.krate.name, "foo_new"); assert_eq!(json.krate.max_version, "1.0.0"); @@ -677,7 +677,7 @@ fn new_krate_with_token() { let (_, _, _, token) = TestApp::full().with_token(); let crate_to_publish = PublishBuilder::new("foo_new").version("1.0.0"); - let json: GoodCrate = token.publish(crate_to_publish).good(); + let json: GoodCrate = token.enqueue_publish(crate_to_publish).good(); assert_eq!(json.krate.name, "foo_new"); assert_eq!(json.krate.max_version, "1.0.0"); @@ -688,7 +688,7 @@ fn new_krate_weird_version() { let (_, _, _, token) = TestApp::full().with_token(); let crate_to_publish = PublishBuilder::new("foo_weird").version("0.0.0-pre"); - let json: GoodCrate = token.publish(crate_to_publish).good(); + let json: GoodCrate = token.enqueue_publish(crate_to_publish).good(); assert_eq!(json.krate.name, "foo_weird"); assert_eq!(json.krate.max_version, "0.0.0-pre"); @@ -708,7 +708,7 @@ fn new_with_renamed_dependency() { let crate_to_publish = PublishBuilder::new("new-krate") .version("1.0.0") .dependency(dependency); - token.publish(crate_to_publish).good(); + token.enqueue_publish(crate_to_publish).good(); app.run_pending_background_jobs(); let crates = app.crates_from_index_head("ne/w-/new-krate"); @@ -737,7 +737,7 @@ fn new_krate_with_dependency() { let crate_to_publish = PublishBuilder::new("new_dep") .version("1.0.0") .dependency(dependency); - token.publish(crate_to_publish).good(); + token.enqueue_publish(crate_to_publish).good(); } #[test] @@ -754,7 +754,7 @@ fn reject_new_krate_with_non_exact_dependency() { let crate_to_publish = PublishBuilder::new("new_dep") .version("1.0.0") .dependency(dependency); - token.publish(crate_to_publish).bad_with_status(200); + token.enqueue_publish(crate_to_publish).bad_with_status(200); } #[test] @@ -767,7 +767,7 @@ fn new_crate_allow_empty_alternative_registry_dependency() { let dependency = DependencyBuilder::new("foo-dep").registry(""); let crate_to_publish = PublishBuilder::new("foo").dependency(dependency); - token.publish(crate_to_publish).good(); + token.enqueue_publish(crate_to_publish).good(); } #[test] @@ -778,7 +778,7 @@ fn reject_new_crate_with_alternative_registry_dependency() { DependencyBuilder::new("dep").registry("https://server.example/path/to/registry"); let crate_to_publish = PublishBuilder::new("depends-on-alt-registry").dependency(dependency); - let json = token.publish(crate_to_publish).bad_with_status(200); + let json = token.enqueue_publish(crate_to_publish).bad_with_status(200); assert!( json.errors[0] .detail @@ -803,7 +803,7 @@ fn new_krate_with_wildcard_dependency() { .version("1.0.0") .dependency(dependency); - let json = token.publish(crate_to_publish).bad_with_status(200); + let json = token.enqueue_publish(crate_to_publish).bad_with_status(200); assert!( json.errors[0].detail.contains("dependency constraints"), "{:?}", @@ -823,7 +823,7 @@ fn new_krate_twice() { let crate_to_publish = PublishBuilder::new("foo_twice") .version("2.0.0") .description("2.0.0 description"); - let json = token.publish(crate_to_publish).good(); + let json = token.enqueue_publish(crate_to_publish).good(); assert_eq!(json.krate.name, "foo_twice"); assert_eq!(json.krate.description.unwrap(), "2.0.0 description"); @@ -842,7 +842,9 @@ fn new_krate_wrong_user() { let another_user = app.db_new_user("another").db_new_token("bar"); let crate_to_publish = PublishBuilder::new("foo_wrong").version("2.0.0"); - let json = another_user.publish(crate_to_publish).bad_with_status(200); + let json = another_user + .enqueue_publish(crate_to_publish) + .bad_with_status(200); assert!( json.errors[0] .detail @@ -870,7 +872,7 @@ fn new_krate_too_big() { let files = [("foo_big-1.0.0/big", &[b'a'; 2000] as &[_])]; let builder = PublishBuilder::new("foo_big").files(&files); - let json = user.publish(builder).bad_with_status(200); + let json = user.enqueue_publish(builder).bad_with_status(200); assert!( json.errors[0] .detail @@ -895,7 +897,7 @@ fn new_krate_too_big_but_whitelisted() { .version("1.1.0") .files(&files); - token.publish(crate_to_publish).good(); + token.enqueue_publish(crate_to_publish).good(); } #[test] @@ -905,7 +907,7 @@ fn new_krate_wrong_files() { let files = [("foo-1.0.0/a", data), ("bar-1.0.0/a", data)]; let builder = PublishBuilder::new("foo").files(&files); - let json = user.publish(builder).bad_with_status(200); + let json = user.enqueue_publish(builder).bad_with_status(200); assert!( json.errors[0].detail.contains("invalid tarball uploaded"), "{:?}", @@ -924,7 +926,7 @@ fn new_krate_gzip_bomb() { .version("1.1.0") .files_with_io(&mut [("foo-1.1.0/a", &mut body, len)]); - let json = token.publish(crate_to_publish).bad_with_status(200); + let json = token.enqueue_publish(crate_to_publish).bad_with_status(200); assert!( json.errors[0] @@ -947,7 +949,7 @@ fn new_krate_duplicate_version() { }); let crate_to_publish = PublishBuilder::new("foo_dupe").version("1.0.0"); - let json = token.publish(crate_to_publish).bad_with_status(200); + let json = token.enqueue_publish(crate_to_publish).bad_with_status(200); assert!( json.errors[0].detail.contains("already uploaded"), @@ -967,7 +969,7 @@ fn new_crate_similar_name() { }); let crate_to_publish = PublishBuilder::new("foo_similar").version("1.1.0"); - let json = token.publish(crate_to_publish).bad_with_status(200); + let json = token.enqueue_publish(crate_to_publish).bad_with_status(200); assert!( json.errors[0].detail.contains("previously named"), @@ -987,7 +989,7 @@ fn new_crate_similar_name_hyphen() { }); let crate_to_publish = PublishBuilder::new("foo-bar-hyphen").version("1.1.0"); - let json = token.publish(crate_to_publish).bad_with_status(200); + let json = token.enqueue_publish(crate_to_publish).bad_with_status(200); assert!( json.errors[0].detail.contains("previously named"), @@ -1007,7 +1009,7 @@ fn new_crate_similar_name_underscore() { }); let crate_to_publish = PublishBuilder::new("foo_bar_underscore").version("1.1.0"); - let json = token.publish(crate_to_publish).bad_with_status(200); + let json = token.enqueue_publish(crate_to_publish).bad_with_status(200); assert!( json.errors[0].detail.contains("previously named"), @@ -1021,7 +1023,7 @@ fn new_krate_git_upload() { let (app, _, _, token) = TestApp::full().with_token(); let crate_to_publish = PublishBuilder::new("fgt"); - token.publish(crate_to_publish).good(); + token.enqueue_publish(crate_to_publish).good(); app.run_pending_background_jobs(); let crates = app.crates_from_index_head("3/f/fgt"); @@ -1040,9 +1042,9 @@ fn new_krate_git_upload_appends() { let (app, _, _, token) = TestApp::full().with_token(); let crate_to_publish = PublishBuilder::new("FPP").version("0.0.1"); - token.publish(crate_to_publish).good(); + token.enqueue_publish(crate_to_publish).good(); let crate_to_publish = PublishBuilder::new("FPP").version("1.0.0"); - token.publish(crate_to_publish).good(); + token.enqueue_publish(crate_to_publish).good(); app.run_pending_background_jobs(); let crates = app.crates_from_index_head("3/f/fpp"); @@ -1069,7 +1071,7 @@ fn new_krate_git_upload_with_conflicts() { .unwrap(); let crate_to_publish = PublishBuilder::new("foo_conflicts"); - token.publish(crate_to_publish).good(); + token.enqueue_publish(crate_to_publish).good(); } #[test] @@ -1081,7 +1083,7 @@ fn new_krate_dependency_missing() { let dependency = DependencyBuilder::new("bar_missing"); let crate_to_publish = PublishBuilder::new("foo_missing").dependency(dependency); - let json = token.publish(crate_to_publish).bad_with_status(200); + let json = token.enqueue_publish(crate_to_publish).bad_with_status(200); assert!( json.errors[0] .detail @@ -1096,7 +1098,7 @@ fn new_krate_with_readme() { let (_, _, _, token) = TestApp::full().with_token(); let crate_to_publish = PublishBuilder::new("foo_readme").readme(""); - let json = token.publish(crate_to_publish).good(); + let json = token.enqueue_publish(crate_to_publish).good(); assert_eq!(json.krate.name, "foo_readme"); assert_eq!(json.krate.max_version, "1.0.0"); @@ -1112,7 +1114,7 @@ fn new_krate_without_any_email_fails() { let crate_to_publish = PublishBuilder::new("foo_no_email"); - let json = token.publish(crate_to_publish).bad_with_status(200); + let json = token.enqueue_publish(crate_to_publish).bad_with_status(200); assert!( json.errors[0] @@ -1136,7 +1138,7 @@ fn new_krate_with_unverified_email_fails() { let crate_to_publish = PublishBuilder::new("foo_unverified_email"); - let json = token.publish(crate_to_publish).bad_with_status(200); + let json = token.enqueue_publish(crate_to_publish).bad_with_status(200); assert!( json.errors[0] @@ -1153,7 +1155,7 @@ fn new_krate_records_verified_email() { let crate_to_publish = PublishBuilder::new("foo_verified_email"); - token.publish(crate_to_publish).good(); + token.enqueue_publish(crate_to_publish).good(); app.db(|conn| { let email = versions_published_by::table @@ -1385,7 +1387,7 @@ fn yank() { // Upload a new crate, putting it in the git index let crate_to_publish = PublishBuilder::new("fyk"); - token.publish(crate_to_publish).good(); + token.enqueue_publish(crate_to_publish).good(); app.run_pending_background_jobs(); let crates = app.crates_from_index_head("3/f/fyk"); @@ -1441,7 +1443,7 @@ fn yank_max_version() { // Upload a new crate let crate_to_publish = PublishBuilder::new("fyk_max"); - token.publish(crate_to_publish).good(); + token.enqueue_publish(crate_to_publish).good(); // double check the max version let json = anon.show_crate("fyk_max"); @@ -1449,7 +1451,7 @@ fn yank_max_version() { // add version 2.0.0 let crate_to_publish = PublishBuilder::new("fyk_max").version("2.0.0"); - let json = token.publish(crate_to_publish).good(); + let json = token.enqueue_publish(crate_to_publish).good(); assert_eq!(json.krate.max_version, "2.0.0"); // yank version 1.0.0 @@ -1495,7 +1497,7 @@ fn publish_after_yank_max_version() { // Upload a new crate let crate_to_publish = PublishBuilder::new("fyk_max"); - token.publish(crate_to_publish).good(); + token.enqueue_publish(crate_to_publish).good(); // double check the max version let json = anon.show_crate("fyk_max"); @@ -1509,7 +1511,7 @@ fn publish_after_yank_max_version() { // add version 2.0.0 let crate_to_publish = PublishBuilder::new("fyk_max").version("2.0.0"); - let json = token.publish(crate_to_publish).good(); + let json = token.enqueue_publish(crate_to_publish).good(); assert_eq!(json.krate.max_version, "2.0.0"); // unyank version 1.0.0 @@ -1540,7 +1542,7 @@ fn publish_after_removing_documentation() { let crate_to_publish = PublishBuilder::new("docscrate") .version("0.2.1") .documentation("http://foo.rs"); - let json = token.publish(crate_to_publish).good(); + let json = token.enqueue_publish(crate_to_publish).good(); assert_eq!(json.krate.documentation, Some("http://foo.rs".to_owned())); // Ensure latest version also has the same documentation @@ -1549,7 +1551,7 @@ fn publish_after_removing_documentation() { // 3. Remove the documentation let crate_to_publish = PublishBuilder::new("docscrate").version("0.2.2"); - let json = token.publish(crate_to_publish).good(); + let json = token.enqueue_publish(crate_to_publish).good(); assert_eq!(json.krate.documentation, None); // Ensure latest version no longer has documentation @@ -1562,13 +1564,13 @@ fn bad_keywords() { let (_, _, _, token) = TestApp::init().with_token(); let crate_to_publish = PublishBuilder::new("foo_bad_key").keyword("super-long-keyword-name-oh-no"); - token.publish(crate_to_publish).bad_with_status(200); + token.enqueue_publish(crate_to_publish).bad_with_status(200); let crate_to_publish = PublishBuilder::new("foo_bad_key").keyword("?@?%"); - token.publish(crate_to_publish).bad_with_status(200); + token.enqueue_publish(crate_to_publish).bad_with_status(200); let crate_to_publish = PublishBuilder::new("foo_bad_key").keyword("áccênts"); - token.publish(crate_to_publish).bad_with_status(200); + token.enqueue_publish(crate_to_publish).bad_with_status(200); } #[test] @@ -1582,7 +1584,7 @@ fn good_categories() { }); let crate_to_publish = PublishBuilder::new("foo_good_cat").category("cat1"); - let json = token.publish(crate_to_publish).good(); + let json = token.enqueue_publish(crate_to_publish).good(); assert_eq!(json.krate.name, "foo_good_cat"); assert_eq!(json.krate.max_version, "1.0.0"); @@ -1594,7 +1596,7 @@ fn ignored_categories() { let (_, _, _, token) = TestApp::full().with_token(); let crate_to_publish = PublishBuilder::new("foo_ignored_cat").category("bar"); - let json = token.publish(crate_to_publish).good(); + let json = token.enqueue_publish(crate_to_publish).good(); assert_eq!(json.krate.name, "foo_ignored_cat"); assert_eq!(json.krate.max_version, "1.0.0"); @@ -1615,7 +1617,7 @@ fn good_badges() { let crate_to_publish = PublishBuilder::new("foobadger").badges(badges); - let json = token.publish(crate_to_publish).good(); + let json = token.enqueue_publish(crate_to_publish).good(); assert_eq!(json.krate.name, "foobadger"); assert_eq!(json.krate.max_version, "1.0.0"); @@ -1647,7 +1649,7 @@ fn ignored_badges() { let crate_to_publish = PublishBuilder::new("foo_ignored_badge").badges(badges); - let json = token.publish(crate_to_publish).good(); + let json = token.enqueue_publish(crate_to_publish).good(); assert_eq!(json.krate.name, "foo_ignored_badge"); assert_eq!(json.krate.max_version, "1.0.0"); assert_eq!(json.warnings.invalid_badges.len(), 2); @@ -1843,7 +1845,7 @@ fn author_license_and_description_required() { .unset_description() .unset_authors(); - let json = token.publish(crate_to_publish).bad_with_status(200); + let json = token.enqueue_publish(crate_to_publish).bad_with_status(200); assert!( json.errors[0].detail.contains("author") && json.errors[0].detail.contains("description") @@ -1858,7 +1860,7 @@ fn author_license_and_description_required() { .unset_authors() .author(""); - let json = token.publish(crate_to_publish).bad_with_status(200); + let json = token.enqueue_publish(crate_to_publish).bad_with_status(200); assert!( json.errors[0].detail.contains("author") && json.errors[0].detail.contains("description") @@ -1873,7 +1875,7 @@ fn author_license_and_description_required() { .license_file("foo") .unset_description(); - let json = token.publish(crate_to_publish).bad_with_status(200); + let json = token.enqueue_publish(crate_to_publish).bad_with_status(200); assert!( !json.errors[0].detail.contains("author") && json.errors[0].detail.contains("description") @@ -2092,7 +2094,7 @@ fn new_krate_tarball_with_hard_links() { let crate_to_publish = PublishBuilder::new("foo").version("1.1.0").tarball(tarball); - let json = token.publish(crate_to_publish).bad_with_status(200); + let json = token.enqueue_publish(crate_to_publish).bad_with_status(200); assert!( json.errors[0] diff --git a/src/tests/owners.rs b/src/tests/owners.rs index 2c8f47bddc4..1678bd12652 100644 --- a/src/tests/owners.rs +++ b/src/tests/owners.rs @@ -63,7 +63,7 @@ fn new_crate_owner() { // Create a crate under one user let crate_to_publish = PublishBuilder::new("foo_owner").version("1.0.0"); - token.publish(crate_to_publish).good(); + token.enqueue_publish(crate_to_publish).good(); // Add the second user as an owner let user2 = app.db_new_user("bar"); @@ -81,7 +81,7 @@ fn new_crate_owner() { let crate_to_publish = PublishBuilder::new("foo_owner").version("2.0.0"); user2 .db_new_token("bar_token") - .publish(crate_to_publish) + .enqueue_publish(crate_to_publish) .good(); } diff --git a/src/tests/team.rs b/src/tests/team.rs index 7b946e820ea..17699d547c6 100644 --- a/src/tests/team.rs +++ b/src/tests/team.rs @@ -202,7 +202,7 @@ fn remove_team_as_named_owner() { let user_on_one_team = app.db_new_user(mock_user_on_only_one_team().gh_login); let crate_to_publish = PublishBuilder::new("foo_remove_team").version("2.0.0"); let json = user_on_one_team - .publish(crate_to_publish) + .enqueue_publish(crate_to_publish) .bad_with_status(200); assert!( @@ -264,7 +264,7 @@ fn publish_not_owned() { let crate_to_publish = PublishBuilder::new("foo_not_owned").version("2.0.0"); let json = user_on_one_team - .publish(crate_to_publish) + .enqueue_publish(crate_to_publish) .bad_with_status(200); assert!( @@ -294,7 +294,7 @@ fn publish_owned() { let user_on_one_team = app.db_new_user(mock_user_on_only_one_team().gh_login); let crate_to_publish = PublishBuilder::new("foo_team_owned").version("2.0.0"); - user_on_one_team.publish(crate_to_publish).good(); + user_on_one_team.enqueue_publish(crate_to_publish).good(); } // Test trying to change owners (when only on an owning team) diff --git a/src/tests/util.rs b/src/tests/util.rs index 25f7934355e..4c2fcd9abc2 100644 --- a/src/tests/util.rs +++ b/src/tests/util.rs @@ -329,7 +329,7 @@ pub trait RequestHelper { /// /// Any pending jobs are run when the `TestApp` is dropped to ensure that the test fails unless /// all background tasks complete successfully. - fn publish(&self, publish_builder: PublishBuilder) -> Response { + fn enqueue_publish(&self, publish_builder: PublishBuilder) -> Response { let krate_name = publish_builder.krate_name.clone(); let response = self.put("/api/v1/crates/new", &publish_builder.body()); let callback_on_good = move |json: &GoodCrate| assert_eq!(json.krate.name, krate_name); diff --git a/src/tests/version.rs b/src/tests/version.rs index acf7cd08748..a2bd84c2d14 100644 --- a/src/tests/version.rs +++ b/src/tests/version.rs @@ -140,14 +140,14 @@ fn version_size() { let (_, _, user) = TestApp::full().with_user(); let crate_to_publish = PublishBuilder::new("foo_version_size").version("1.0.0"); - user.publish(crate_to_publish).good(); + user.enqueue_publish(crate_to_publish).good(); // Add a file to version 2 so that it's a different size than version 1 let files = [("foo_version_size-2.0.0/big", &[b'a'; 1] as &[_])]; let crate_to_publish = PublishBuilder::new("foo_version_size") .version("2.0.0") .files(&files); - user.publish(crate_to_publish).good(); + user.enqueue_publish(crate_to_publish).good(); let crate_json = user.show_crate("foo_version_size"); From 119b622ac4208d4d49fb2cfe0858066863b0e54a Mon Sep 17 00:00:00 2001 From: Justin Geibel Date: Mon, 18 Mar 2019 19:21:44 -0400 Subject: [PATCH 3/5] Only initialize an outgoing client if the test is in capture mode There is no reason to initialize this client when in playback mode. --- src/tests/record.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/tests/record.rs b/src/tests/record.rs index c04c2b1b032..4b8ffa96f34 100644 --- a/src/tests/record.rs +++ b/src/tests/record.rs @@ -105,7 +105,11 @@ pub fn proxy() -> (String, Bomb) { let handle = core.handle(); let addr = t!(a.local_addr()); let listener = t!(TcpListener::from_listener(a, &addr, &handle)); - let client = hyper::Client::builder().build(hyper_tls::HttpsConnector::new(4).unwrap()); + let client = if let Record::Capture(_, _) = record { + Some(hyper::Client::builder().build(hyper_tls::HttpsConnector::new(4).unwrap())) + } else { + None + }; let record = Arc::new(Mutex::new(record)); let srv = hyper::Server::builder(listener.incoming().map(|(l, _)| l)) @@ -142,7 +146,7 @@ pub fn proxy() -> (String, Bomb) { struct Proxy { sink: Sink, record: Arc>, - client: Client, + client: Option, } impl hyper::service::Service for Proxy { @@ -155,7 +159,7 @@ impl hyper::service::Service for Proxy { fn call(&mut self, req: hyper::Request) -> Self::Future { let record2 = self.record.clone(); match *self.record.lock().unwrap() { - Record::Capture(_, _) => Box::new(record_http(req, &self.client).map( + Record::Capture(_, _) => Box::new(record_http(req, self.client.as_ref().unwrap()).map( move |(response, exchange)| { if let Record::Capture(ref mut d, _) = *record2.lock().unwrap() { d.push(exchange); From 3fd1bfb72bf82804772b6048f609d7dd5c7063fa Mon Sep 17 00:00:00 2001 From: Justin Geibel Date: Mon, 18 Mar 2019 21:53:48 -0400 Subject: [PATCH 4/5] Switch to no proxy by default for tests The remaining 12 tests that call `app()` directly do not need a proxy. --- src/tests/all.rs | 13 +++++++++---- src/tests/category.rs | 2 +- src/tests/owners.rs | 4 ++-- src/tests/user.rs | 16 ++++++++-------- src/tests/util.rs | 10 +++++----- 5 files changed, 25 insertions(+), 20 deletions(-) diff --git a/src/tests/all.rs b/src/tests/all.rs index 2d449729953..603c4512b64 100644 --- a/src/tests/all.rs +++ b/src/tests/all.rs @@ -108,17 +108,22 @@ pub struct OkBool { ok: bool, } -fn app() -> ( +/// Initialize the app and a proxy that can record and playback outgoing HTTP requests +fn app_with_proxy() -> ( record::Bomb, Arc, conduit_middleware::MiddlewareBuilder, ) { let (proxy, bomb) = record::proxy(); - let (app, handler) = simple_app(Some(proxy)); + let (app, handler) = init_app(Some(proxy)); (bomb, app, handler) } -fn simple_app(proxy: Option) -> (Arc, conduit_middleware::MiddlewareBuilder) { +fn app() -> (Arc, conduit_middleware::MiddlewareBuilder) { + init_app(None) +} + +fn init_app(proxy: Option) -> (Arc, conduit_middleware::MiddlewareBuilder) { let uploader = Uploader::S3 { bucket: s3::Bucket::new( String::from("alexcrichton-test"), @@ -287,7 +292,7 @@ fn logout(req: &mut dyn Request) { fn multiple_live_references_to_the_same_connection_can_be_checked_out() { use std::ptr; - let (_bomb, app, _) = app(); + let (app, _) = app(); let conn1 = app.diesel_database.get().unwrap(); let conn2 = app.diesel_database.get().unwrap(); let conn1_ref: &PgConnection = &conn1; diff --git a/src/tests/category.rs b/src/tests/category.rs index 99f4ad0f57c..14dd2cec0f6 100644 --- a/src/tests/category.rs +++ b/src/tests/category.rs @@ -76,7 +76,7 @@ fn show() { #[test] #[allow(clippy::cyclomatic_complexity)] fn update_crate() { - let (_b, app, middle) = app(); + let (app, middle) = app(); let mut req = req(Method::Get, "/api/v1/categories/foo"); macro_rules! cnt { ($req:expr, $cat:expr) => {{ diff --git a/src/tests/owners.rs b/src/tests/owners.rs index 1678bd12652..c3728fd3b82 100644 --- a/src/tests/owners.rs +++ b/src/tests/owners.rs @@ -233,7 +233,7 @@ fn test_accept_invitation() { crate_owner_invitation: InvitationResponse, } - let (_b, app, middle) = app(); + let (app, middle) = app(); let mut req = req(Method::Get, "/api/v1/me/crate_owner_invitations"); let (krate, user) = { let conn = app.diesel_database.get().unwrap(); @@ -313,7 +313,7 @@ fn test_decline_invitation() { crate_owner_invitation: InvitationResponse, } - let (_b, app, middle) = app(); + let (app, middle) = app(); let mut req = req(Method::Get, "/api/v1/me/crate_owner_invitations"); let (krate, user) = { let conn = app.diesel_database.get().unwrap(); diff --git a/src/tests/user.rs b/src/tests/user.rs index 945b15f6ce5..6df4a311589 100644 --- a/src/tests/user.rs +++ b/src/tests/user.rs @@ -293,7 +293,7 @@ fn updating_existing_user_doesnt_change_api_token() { */ #[test] fn test_github_login_does_not_overwrite_email() { - let (_b, app, middle) = app(); + let (app, middle) = app(); let mut req = req(Method::Get, "/api/v1/me"); let user = { let conn = app.diesel_database.get().unwrap(); @@ -343,7 +343,7 @@ fn test_github_login_does_not_overwrite_email() { */ #[test] fn test_email_get_and_put() { - let (_b, app, middle) = app(); + let (app, middle) = app(); let mut req = req(Method::Get, "/api/v1/me"); let user = { let conn = app.diesel_database.get().unwrap(); @@ -386,7 +386,7 @@ fn test_email_get_and_put() { */ #[test] fn test_empty_email_not_added() { - let (_b, app, middle) = app(); + let (app, middle) = app(); let mut req = req(Method::Get, "/api/v1/me"); let user = { let conn = app.diesel_database.get().unwrap(); @@ -434,7 +434,7 @@ fn test_empty_email_not_added() { */ #[test] fn test_this_user_cannot_change_that_user_email() { - let (_b, app, middle) = app(); + let (app, middle) = app(); let mut req = req(Method::Get, "/api/v1/me"); let not_signed_in_user = { @@ -471,7 +471,7 @@ fn test_this_user_cannot_change_that_user_email() { */ #[test] fn test_insert_into_email_table() { - let (_b, app, middle) = app(); + let (app, middle) = app(); let mut req = req(Method::Get, "/me"); let user = { let conn = app.diesel_database.get().unwrap(); @@ -518,7 +518,7 @@ fn test_insert_into_email_table() { */ #[test] fn test_insert_into_email_table_with_email_change() { - let (_b, app, middle) = app(); + let (app, middle) = app(); let mut req = req(Method::Get, "/me"); let user = { let conn = app.diesel_database.get().unwrap(); @@ -581,7 +581,7 @@ fn test_insert_into_email_table_with_email_change() { fn test_confirm_user_email() { use cargo_registry::schema::emails; - let (_b, app, middle) = app(); + let (app, middle) = app(); let mut req = req(Method::Get, "/me"); let user = { let conn = app.diesel_database.get().unwrap(); @@ -627,7 +627,7 @@ fn test_existing_user_email() { use chrono::NaiveDateTime; use diesel::update; - let (_b, app, middle) = app(); + let (app, middle) = app(); let mut req = req(Method::Get, "/me"); { let conn = app.diesel_database.get().unwrap(); diff --git a/src/tests/util.rs b/src/tests/util.rs index 4c2fcd9abc2..2c6bb560e21 100644 --- a/src/tests/util.rs +++ b/src/tests/util.rs @@ -20,8 +20,8 @@ //! to the underlying database model value (`User` and `ApiToken` respectively). use crate::{ - app, builders::PublishBuilder, record, CrateList, CrateResponse, GoodCrate, OkBool, - VersionResponse, + app, app_with_proxy, builders::PublishBuilder, record, CrateList, CrateResponse, GoodCrate, + OkBool, VersionResponse, }; use cargo_registry::{ background_jobs::Environment, @@ -93,7 +93,7 @@ pub struct TestApp(Rc); impl TestApp { /// Initialize an application with an `Uploader` that panics pub fn init() -> TestAppBuilder { - let (app, middle) = crate::simple_app(None); + let (app, middle) = app(); let inner = Rc::new(TestAppInner { app, _bomb: None, @@ -106,7 +106,7 @@ impl TestApp { /// Initialize the app and a proxy that can record and playback outgoing HTTP requests pub fn with_proxy() -> TestAppBuilder { - let (bomb, app, middle) = app(); + let (bomb, app, middle) = app_with_proxy(); let inner = Rc::new(TestAppInner { app, _bomb: Some(bomb), @@ -121,7 +121,7 @@ impl TestApp { pub fn full() -> TestAppBuilder { use crate::git; - let (bomb, app, middle) = app(); + let (bomb, app, middle) = app_with_proxy(); git::init(); let thread_local_path = git::bare(); From 85ee13e03aab0a5c17999e49b938f4b5528e70f1 Mon Sep 17 00:00:00 2001 From: Justin Geibel Date: Wed, 20 Mar 2019 19:22:49 -0400 Subject: [PATCH 5/5] Avoid initializing the playback proxy for a few remaining tests This list was obtained by temporarily causing playback to panic if there was no `http-data` file when the proxy is started. The only remaining test that starts a proxy server which goes unused is `krate::yank_not_owner`, however this test does need an index. If more tests fall into this category over time we can switch to a full builder API for orchestrating a `TestApp` but that doesn't seem necessary yet. --- src/tests/krate.rs | 8 ++++---- src/tests/read_only_mode.rs | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/tests/krate.rs b/src/tests/krate.rs index b52b2e56916..6bd2afeda03 100644 --- a/src/tests/krate.rs +++ b/src/tests/krate.rs @@ -1106,7 +1106,7 @@ fn new_krate_with_readme() { #[test] fn new_krate_without_any_email_fails() { - let (app, _, _, token) = TestApp::with_proxy().with_token(); + let (app, _, _, token) = TestApp::init().with_token(); app.db(|conn| { delete(emails::table).execute(conn).unwrap(); @@ -1127,7 +1127,7 @@ fn new_krate_without_any_email_fails() { #[test] fn new_krate_with_unverified_email_fails() { - let (app, _, _, token) = TestApp::with_proxy().with_token(); + let (app, _, _, token) = TestApp::init().with_token(); app.db(|conn| { update(emails::table) @@ -1241,7 +1241,7 @@ fn summary_new_crates() { #[test] fn download() { use chrono::{Duration, Utc}; - let (app, anon, user) = TestApp::with_proxy().with_user(); + let (app, anon, user) = TestApp::init().with_user(); let user = user.as_model(); app.db(|conn| { @@ -1293,7 +1293,7 @@ fn download() { #[test] fn download_nonexistent_version_of_existing_crate_404s() { - let (app, anon, user) = TestApp::with_proxy().with_user(); + let (app, anon, user) = TestApp::init().with_user(); let user = user.as_model(); app.db(|conn| { diff --git a/src/tests/read_only_mode.rs b/src/tests/read_only_mode.rs index de034101c4e..3da27d815a0 100644 --- a/src/tests/read_only_mode.rs +++ b/src/tests/read_only_mode.rs @@ -32,7 +32,7 @@ fn cannot_hit_endpoint_which_writes_db_in_read_only_mode() { #[test] fn can_download_crate_in_read_only_mode() { - let (app, anon, user) = TestApp::with_proxy().with_user(); + let (app, anon, user) = TestApp::init().with_user(); app.db(|conn| { CrateBuilder::new("foo_download_read_only", user.as_model().id)