Merge rust-lang#1519

1519: Add `TestApp::simple()` for tests that don't need HTTP recording r=jtgeibel a=jtgeibel

The unused `Uploader::NoOp` option is now `Uploader::Panic` which
panics on any usage.  The panic message directs users to initialize the
app with `TestApp::with_proxy()`.

While this adds more panics to the main lib, the application
configuration isn't dynamic and it should be obvious that
`Uploader::Panic` is not to be used to configure a production
application.

Co-authored-by: Justin Geibel <jtgeibel@gmail.com>

src/app.rs

@@ -103,8 +103,8 @@ impl App {
     /// Returns a client for making HTTP requests to upload crate files.
     ///
     /// The handle will go through a proxy if the uploader being used has specified one, which
-    /// is only done in test mode in order to be able to record and inspect the HTTP requests
-    /// that tests make.
+    /// is only done in tests with `TestApp::with_proxy()` in order to be able to record and
+    /// inspect the HTTP requests that tests make.
     pub fn http_client(&self) -> CargoResult<reqwest::Client> {
         let mut builder = reqwest::Client::builder();
         if let Some(proxy) = self.config.uploader.proxy() {

src/tests/all.rs

@@ -135,26 +135,30 @@ fn app() -> (
     conduit_middleware::MiddlewareBuilder,
 ) {
     dotenv::dotenv().ok();
-    git::init();
 
     let (proxy, bomb) = record::proxy();
-
-    // When testing we route all API traffic over HTTP so we can
-    // sniff/record it, but everywhere else we use https
-    let api_protocol = String::from("http");
-
     let uploader = cargo_registry::Uploader::S3 {
         bucket: s3::Bucket::new(
             String::from("alexcrichton-test"),
             None,
             std::env::var("S3_ACCESS_KEY").unwrap_or_default(),
             std::env::var("S3_SECRET_KEY").unwrap_or_default(),
-            &api_protocol,
+            // When testing we route all API traffic over HTTP so we can
+            // sniff/record it, but everywhere else we use https
+            "http",
         ),
         proxy: Some(proxy),
         cdn: None,
     };
 
+    let (app, handler) = simple_app(uploader);
+    (bomb, app, handler)
+}
+
+fn simple_app(
+    uploader: cargo_registry::Uploader,
+) -> (Arc<App>, conduit_middleware::MiddlewareBuilder) {
+    git::init();
     let config = cargo_registry::Config {
         uploader,
         session_key: "test this has to be over 32 bytes long".to_string(),
@@ -166,13 +170,15 @@ fn app() -> (
         max_upload_size: 3000,
         max_unpack_size: 2000,
         mirror: Replica::Primary,
-        api_protocol,
+        // When testing we route all API traffic over HTTP so we can
+        // sniff/record it, but everywhere else we use https
+        api_protocol: String::from("http"),
     };
     let app = App::new(&config);
     t!(t!(app.diesel_database.get()).begin_test_transaction());
     let app = Arc::new(app);
     let handler = cargo_registry::build_handler(Arc::clone(&app));
-    (bomb, app, handler)
+    (app, handler)
 }
 
 // Return the environment variable only if it has been defined

src/tests/category.rs

@@ -24,7 +24,7 @@ struct CategoryWithSubcategories {
 
 #[test]
 fn index() {
-    let (app, anon) = TestApp::empty();
+    let (app, anon) = TestApp::init().empty();
     let url = "/api/v1/categories";
 
     // List 0 categories if none exist
@@ -51,7 +51,7 @@ fn index() {
 
 #[test]
 fn show() {
-    let (app, anon) = TestApp::empty();
+    let (app, anon) = TestApp::init().empty();
     let url = "/api/v1/categories/foo-bar";
 
     // Return not found if a category doesn't exist
@@ -161,7 +161,7 @@ fn update_crate() {
 
 #[test]
 fn category_slugs_returns_all_slugs_in_alphabetical_order() {
-    let (app, anon) = TestApp::empty();
+    let (app, anon) = TestApp::init().empty();
     app.db(|conn| {
         new_category("Foo", "foo", "For crates that foo")
             .create_or_update(&conn)

src/tests/keyword.rs

@@ -19,7 +19,7 @@ struct GoodKeyword {
 #[test]
 fn index() {
     let url = "/api/v1/keywords";
-    let (app, anon) = TestApp::empty();
+    let (app, anon) = TestApp::init().empty();
     let json: KeywordList = anon.get(url).good();
     assert_eq!(json.keywords.len(), 0);
     assert_eq!(json.meta.total, 0);
@@ -37,7 +37,7 @@ fn index() {
 #[test]
 fn show() {
     let url = "/api/v1/keywords/foo";
-    let (app, anon) = TestApp::empty();
+    let (app, anon) = TestApp::init().empty();
     anon.get(url).assert_not_found();
 
     app.db(|conn| {
@@ -50,7 +50,7 @@ fn show() {
 #[test]
 fn uppercase() {
     let url = "/api/v1/keywords/UPPER";
-    let (app, anon) = TestApp::empty();
+    let (app, anon) = TestApp::init().empty();
     anon.get(url).assert_not_found();
 
     app.db(|conn| {
@@ -62,7 +62,7 @@ fn uppercase() {
 
 #[test]
 fn update_crate() {
-    let (app, anon) = TestApp::empty();
+    let (app, anon) = TestApp::init().empty();
     let cnt = |kw: &str| {
         let json: GoodKeyword = anon.get(&format!("/api/v1/keywords/{}", kw)).good();
         json.keyword.crates_cnt as usize

src/tests/krate.rs

@@ -79,7 +79,7 @@ impl ::util::MockTokenUser {
 
 #[test]
 fn index() {
-    let (app, anon) = TestApp::empty();
+    let (app, anon) = TestApp::init().empty();
     let json = anon.search("");
     assert_eq!(json.crates.len(), 0);
     assert_eq!(json.meta.total, 0);
@@ -98,7 +98,7 @@ fn index() {
 
 #[test]
 fn index_queries() {
-    let (app, anon, user) = TestApp::with_user();
+    let (app, anon, user) = TestApp::init().with_user();
     let user = user.as_model();
 
     let (krate, krate2) = app.db(|conn| {
@@ -179,7 +179,7 @@ fn index_queries() {
 
 #[test]
 fn search_includes_crates_where_name_is_stopword() {
-    let (app, anon, user) = TestApp::with_user();
+    let (app, anon, user) = TestApp::init().with_user();
     let user = user.as_model();
     app.db(|conn| {
         CrateBuilder::new("which", user.id).expect_build(conn);
@@ -194,7 +194,7 @@ fn search_includes_crates_where_name_is_stopword() {
 
 #[test]
 fn exact_match_first_on_queries() {
-    let (app, anon, user) = TestApp::with_user();
+    let (app, anon, user) = TestApp::init().with_user();
     let user = user.as_model();
 
     app.db(|conn| {
@@ -236,7 +236,7 @@ fn exact_match_first_on_queries() {
 
 #[test]
 fn index_sorting() {
-    let (app, anon, user) = TestApp::with_user();
+    let (app, anon, user) = TestApp::init().with_user();
     let user = user.as_model();
 
     app.db(|conn| {
@@ -318,7 +318,7 @@ fn index_sorting() {
 
 #[test]
 fn exact_match_on_queries_with_sort() {
-    let (app, anon, user) = TestApp::with_user();
+    let (app, anon, user) = TestApp::init().with_user();
     let user = user.as_model();
 
     app.db(|conn| {
@@ -416,7 +416,7 @@ fn exact_match_on_queries_with_sort() {
 
 #[test]
 fn show() {
-    let (app, anon, user) = TestApp::with_user();
+    let (app, anon, user) = TestApp::init().with_user();
     let user = user.as_model();
 
     let krate = app.db(|conn| {
@@ -463,7 +463,7 @@ fn show() {
 
 #[test]
 fn yanked_versions_are_not_considered_for_max_version() {
-    let (app, anon, user) = TestApp::with_user();
+    let (app, anon, user) = TestApp::init().with_user();
     let user = user.as_model();
 
     app.db(|conn| {
@@ -481,7 +481,7 @@ fn yanked_versions_are_not_considered_for_max_version() {
 
 #[test]
 fn versions() {
-    let (app, anon) = TestApp::empty();
+    let (app, anon) = TestApp::init().empty();
     app.db(|conn| {
         let u = new_user("foo").create_or_update(conn).unwrap();
         CrateBuilder::new("foo_versions", u.id)
@@ -854,7 +854,7 @@ fn valid_feature_names() {
 
 #[test]
 fn new_krate_too_big() {
-    let (_, _, user) = TestApp::with_user();
+    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);
 
@@ -1103,13 +1103,13 @@ fn new_krate_with_readme() {
 
 #[test]
 fn summary_doesnt_die() {
-    let (_, anon) = TestApp::empty();
+    let (_, anon) = TestApp::init().empty();
     anon.get::<SummaryResponse>("/api/v1/summary").good();
 }
 
 #[test]
 fn summary_new_crates() {
-    let (app, anon, user) = TestApp::with_user();
+    let (app, anon, user) = TestApp::init().with_user();
     let user = user.as_model();
     app.db(|conn| {
         let krate = CrateBuilder::new("some_downloads", user.id)
@@ -1176,7 +1176,7 @@ fn summary_new_crates() {
 #[test]
 fn download() {
     use chrono::{Duration, Utc};
-    let (app, anon, user) = TestApp::with_user();
+    let (app, anon, user) = TestApp::with_proxy().with_user();
     let user = user.as_model();
 
     app.db(|conn| {
@@ -1260,7 +1260,7 @@ fn dependencies() {
 
 #[test]
 fn diesel_not_found_results_in_404() {
-    let (_, _, user) = TestApp::with_user();
+    let (_, _, user) = TestApp::init().with_user();
 
     user.get("/api/v1/crates/foo_following/following")
         .assert_not_found();
@@ -1269,7 +1269,7 @@ fn diesel_not_found_results_in_404() {
 #[test]
 fn following() {
     // TODO: Test anon requests as well?
-    let (app, _, user) = TestApp::with_user();
+    let (app, _, user) = TestApp::init().with_user();
 
     app.db(|conn| {
         CrateBuilder::new("foo_following", user.as_model().id).expect_build(&conn);
@@ -1399,7 +1399,7 @@ fn yank() {
 
 #[test]
 fn yank_not_owner() {
-    let (app, _, _, token) = TestApp::with_token();
+    let (app, _, _, token) = TestApp::init().with_token();
     app.db(|conn| {
         let another_user = new_user("bar").create_or_update(conn).unwrap();
         CrateBuilder::new("foo_not", another_user.id).expect_build(conn);
@@ -2019,7 +2019,7 @@ fn author_license_and_description_required() {
 */
 #[test]
 fn test_recent_download_count() {
-    let (app, anon, user) = TestApp::with_user();
+    let (app, anon, user) = TestApp::init().with_user();
     let user = user.as_model();
 
     app.db(|conn| {
@@ -2057,7 +2057,7 @@ fn test_recent_download_count() {
  */
 #[test]
 fn test_zero_downloads() {
-    let (app, anon, user) = TestApp::with_user();
+    let (app, anon, user) = TestApp::init().with_user();
     let user = user.as_model();
 
     app.db(|conn| {
@@ -2082,7 +2082,7 @@ fn test_zero_downloads() {
 */
 #[test]
 fn test_default_sort_recent() {
-    let (app, anon, user) = TestApp::with_user();
+    let (app, anon, user) = TestApp::init().with_user();
     let user = user.as_model();
 
     let (green_crate, potato_crate) = app.db(|conn| {
@@ -2145,7 +2145,7 @@ fn test_default_sort_recent() {
 
 #[test]
 fn block_bad_documentation_url() {
-    let (app, anon, user) = TestApp::with_user();
+    let (app, anon, user) = TestApp::init().with_user();
     let user = user.as_model();
 
     app.db(|conn| {
@@ -2163,7 +2163,7 @@ fn block_bad_documentation_url() {
 // which call the `PUT /crates/:crate_id/owners` route
 #[test]
 fn test_cargo_invite_owners() {
-    let (app, _, owner) = TestApp::with_user();
+    let (app, _, owner) = TestApp::init().with_user();
 
     let new_user = app.db_new_user("cilantro");
     app.db(|conn| {

src/tests/owners.rs

@@ -63,7 +63,7 @@ impl ::util::MockCookieUser {
 
 #[test]
 fn new_crate_owner() {
-    let (app, _, user1, token) = TestApp::with_token();
+    let (app, _, user1, token) = TestApp::with_proxy().with_token();
 
     // Create a crate under one user
     let crate_to_publish = PublishBuilder::new("foo_owner").version("1.0.0");
@@ -203,7 +203,7 @@ fn owners_can_remove_self() {
 */
 #[test]
 fn check_ownership_two_crates() {
-    let (app, anon, user) = TestApp::with_user();
+    let (app, anon, user) = TestApp::init().with_user();
     let user = user.as_model();
 
     let (krate_owned_by_team, team) = app.db(|conn| {