Begin working on a revamped test harness

This is yet another attempt to improve our test harness. This is
intended to be a replacement for `TestApp` which currently lives in
`tests/util.rs`

This new structure tries to address several pain points I've had using
the existing harness:

- Abstracting into smaller pieces

  Most of the functions in the current harness give no opportunity to
  modify the request, require a type to parse the response into, and
  assume the request is a 2xx or 3xx response. This has made testing our
  edge cases difficult, and they're less well tested as a result. I've
  attempted to make it easier to do: "just like this other thing but
  with an additional header set", or "I don't care about the response
  body"

  What was previously `anon.get::<T>("...")` will now be 3 separate
  pieces:
  - `app.get("...")` returns a request object that can be modified if
    needed
  - `request.send()` returns a `Result`, where a 4xx or 5xx response is
    considered an error
  - `response.json<T>()` will parse the body into the given type

  The API is roughly modeled after `reqwest`, which I've enjoyed using
  in our app.

- More explicit handling of auth

  The current API for auth has felt really funky to me. Having an object
  called `User` constructing HTTP requests felt backwards, and it also
  meant we had to decide up front whether we were going to send some
  requests signed in or not, and also how we were authenticating. It
  also didn't have a good way to send requests as more than one user.

  The new API moves auth to be a part of request construction. So
  `user.get("...")` becomes `app.get("...").as_user(&user)`

Finally, I've structured this new API around returning `Result`
everywhere, so that we can keep assertions closer to the tests where
they matter (giving line numbers in the test, not the util file). This
is a bit tricky, and I'll probably be tweaking as I move tests over,
since results bubbled all the way up with `?` will completely lose the
backtrace. Right now, I'm figuring that things we really just don't
expect to fail ever get bubbled up with `?`, while the things we're
actually testing will remain as assertions.

The tests I've moved over don't have a ton to abstract, but I'd also
like to keep helpers like `publish` off of the `App` struct, and instead
keep them in the test file where they're relevant.

I've started by just moving `tests/read_only_mode` over, since it's a
relatively small file, and also one where the old API was more painful
for me to use. I'll be porting other files over in the next few days.

Author: sgrif

src/tests/all.rs

@@ -61,6 +61,9 @@ macro_rules! bad_resp {
     }};
 }
 
+mod helpers;
+mod prelude;
+
 mod badge;
 mod builders;
 mod categories;

src/tests/builders.rs

@@ -229,7 +229,7 @@ impl<'a> CrateBuilder<'a> {
         self
     }
 
-    fn build(mut self, connection: &PgConnection) -> CargoResult<Crate> {
+    pub fn build(mut self, connection: &PgConnection) -> CargoResult<Crate> {
         use diesel::{insert_into, select, update};
 
         let mut krate = self

src/tests/helpers.rs

@@ -0,0 +1,3 @@
+pub mod app;
+pub mod request;
+pub mod response;

src/tests/helpers/app.rs

@@ -0,0 +1,78 @@
+use super::request::RequestBuilder;
+use cargo_registry::models::{ApiToken, NewUser, User};
+use cargo_registry::util::CargoResult;
+use conduit::Method;
+use conduit_middleware::MiddlewareBuilder;
+use diesel::prelude::*;
+use std::sync::Arc;
+
+pub struct App {
+    app: Arc<cargo_registry::App>,
+    middleware: MiddlewareBuilder,
+}
+
+impl App {
+    pub fn new() -> Self {
+        let (app, middleware) = crate::simple_app(None);
+        Self { app, middleware }
+    }
+
+    /// Obtain the database connection and pass it to the closure
+    ///
+    /// Our tests share a database connection with the app server, so it's
+    /// important that the conenction is dropped before requests are made to
+    /// ensure it's available for the server to use. The connection will be
+    /// returned to the server after the given function returns.
+    pub fn db<T, F>(&self, f: F) -> CargoResult<T>
+    where
+        F: FnOnce(&PgConnection) -> CargoResult<T>,
+    {
+        let conn = self.app.diesel_database.get()?;
+        f(&conn)
+    }
+
+    /// Create a new user in the database with the given id
+    pub fn create_user(&self, username: &str) -> CargoResult<User> {
+        self.db(|conn| {
+            let new_user = NewUser {
+                email: Some("[email protected]"),
+                ..crate::new_user(username)
+            };
+            Ok(new_user.create_or_update(conn)?)
+        })
+    }
+
+    /// Sets the database in read only mode.
+    ///
+    /// Any attempts to modify the database after calling this function will
+    /// result in an error.
+    pub fn set_read_only(&self) -> CargoResult<()> {
+        self.db(|conn| {
+            diesel::sql_query("SET TRANSACTION READ ONLY").execute(conn)?;
+            Ok(())
+        })
+    }
+
+    /// Create an HTTP `GET` request
+    pub fn get(&self, path: &str) -> RequestBuilder<'_> {
+        RequestBuilder::new(&self.middleware, Method::Get, path)
+    }
+
+    /// Create an HTTP `DELETE` request
+    pub fn delete(&self, path: &str) -> RequestBuilder<'_> {
+        RequestBuilder::new(&self.middleware, Method::Delete, path)
+    }
+
+    /// Returns the first API token for the given user, or creates a new one
+    pub fn token_for(&self, user: &User) -> CargoResult<ApiToken> {
+        self.db(|conn| {
+            ApiToken::belonging_to(user)
+                .first(conn)
+                .optional()?
+                .map(Ok)
+                .unwrap_or_else(|| {
+                    ApiToken::insert(conn, user.id, "test_token").map_err(Into::into)
+                })
+        })
+    }
+}

src/tests/helpers/request.rs

@@ -0,0 +1,38 @@
+use super::response::{Response, ResponseError};
+use cargo_registry::models::ApiToken;
+use conduit::{Handler, Method};
+use conduit_middleware::MiddlewareBuilder;
+use conduit_test::MockRequest;
+
+pub struct RequestBuilder<'a> {
+    middleware: &'a MiddlewareBuilder,
+    request: MockRequest,
+}
+
+impl<'a> RequestBuilder<'a> {
+    pub(super) fn new(middleware: &'a MiddlewareBuilder, method: Method, path: &str) -> Self {
+        Self {
+            middleware,
+            request: MockRequest::new(method, path),
+        }
+        .with_header("User-Agent", "conduit-test")
+    }
+
+    /// Uses the given token for authentication
+    pub fn with_token(self, token: &ApiToken) -> Self {
+        self.with_header("Authorization", &token.token)
+    }
+
+    pub fn with_header(mut self, name: &str, value: &str) -> Self {
+        self.request.header(name, value);
+        self
+    }
+
+    /// Send the request
+    ///
+    /// Returns an error if any of the middlewares returned an error, or if
+    /// the response status was >= 400.
+    pub fn send(mut self) -> Result<Response, ResponseError> {
+        Response::new(self.middleware.call(&mut self.request)?)
+    }
+}

src/tests/helpers/response.rs

@@ -0,0 +1,86 @@
+use cargo_registry::util::{CargoError, CargoResult};
+use std::error::Error;
+use std::fmt;
+
+pub struct Response {
+    inner: conduit::Response,
+    body: String,
+}
+
+impl Response {
+    pub(super) fn new(mut inner: conduit::Response) -> Result<Self, ResponseError> {
+        use ResponseError::*;
+
+        let mut body = Vec::new();
+        inner.body.write_body(&mut body).unwrap();
+
+        let resp = Response {
+            inner,
+            body: String::from_utf8(body).unwrap(),
+        };
+
+        match resp.status() {
+            400...499 => Err(BadRequest(resp)),
+            500...599 => Err(ServerError(resp)),
+            _ => Ok(resp),
+        }
+    }
+
+    pub fn status(&self) -> u32 {
+        self.inner.status.0
+    }
+
+    pub fn text(&self) -> &str {
+        &self.body
+    }
+}
+
+pub enum ResponseError {
+    MiddlewareError(Box<dyn CargoError>),
+    BadRequest(Response),
+    ServerError(Response),
+}
+
+impl From<Box<dyn Error + Send>> for ResponseError {
+    fn from(e: Box<dyn Error + Send>) -> Self {
+        ResponseError::MiddlewareError(CargoError::from_std_error(e))
+    }
+}
+
+impl fmt::Debug for ResponseError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        use ResponseError::*;
+        match self {
+            MiddlewareError(e) => write!(f, "MiddlewareError({:?})", e),
+            BadRequest(_) => write!(f, "BadRequest(_)"),
+            ServerError(_) => write!(f, "ServerError(_)"),
+        }
+    }
+}
+
+impl fmt::Display for ResponseError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        use ResponseError::*;
+        match self {
+            MiddlewareError(e) => write!(f, "middleware error: {}", e),
+            BadRequest(e) => write!(f, "bad request: {}", e.text()),
+            ServerError(e) => write!(f, "server error: {}", e.text()),
+        }
+    }
+}
+
+impl Error for ResponseError {}
+
+pub trait ResultExt {
+    fn allow_errors(self) -> CargoResult<Response>;
+}
+
+impl ResultExt for Result<Response, ResponseError> {
+    fn allow_errors(self) -> CargoResult<Response> {
+        use ResponseError::*;
+        self.or_else(|e| match e {
+            MiddlewareError(e) => Err(e),
+            BadRequest(r) | ServerError(r) => Ok(r),
+        })
+    }
+}

src/tests/prelude.rs

@@ -0,0 +1,3 @@
+pub use crate::helpers::app::*;
+pub use crate::helpers::response::ResultExt as _;
+pub use cargo_registry::util::CargoResult;

src/tests/read_only_mode.rs

@@ -1,41 +1,52 @@
 use crate::builders::CrateBuilder;
-use crate::{RequestHelper, TestApp};
+use crate::prelude::*;
 use diesel::prelude::*;
 
 #[test]
-fn can_hit_read_only_endpoints_in_read_only_mode() {
-    let (app, anon) = TestApp::init().empty();
-    app.db(set_read_only).unwrap();
-    anon.get::<()>("/api/v1/crates").assert_status(200);
+fn can_hit_read_only_endpoints_in_read_only_mode() -> CargoResult<()> {
+    let app = App::new();
+    app.set_read_only()?;
+    app.get("/api/v1/crates").send()?;
+    Ok(())
 }
 
 #[test]
-fn cannot_hit_endpoint_which_writes_db_in_read_only_mode() {
-    let (app, _, user, token) = TestApp::init().with_token();
+fn cannot_hit_endpoint_which_writes_db_in_read_only_mode() -> CargoResult<()> {
+    let app = App::new();
+    let user = app.create_user("new_user")?;
+    let token = app.token_for(&user)?;
+
     app.db(|conn| {
-        CrateBuilder::new("foo_yank_read_only", user.as_model().id)
+        CrateBuilder::new("foo_yank_read_only", user.id)
             .version("1.0.0")
-            .expect_build(conn);
-        set_read_only(conn).unwrap();
-    });
-    token
-        .delete::<()>("/api/v1/crates/foo_yank_read_only/1.0.0/yank")
-        .assert_status(503);
+            .build(conn)
+    })?;
+    app.set_read_only()?;
+    let resp = app
+        .delete("/api/v1/crates/foo_yank_read_only/1.0.0/yank")
+        .with_token(&token)
+        .send()
+        .allow_errors()?;
+    assert_eq!(503, resp.status());
+    Ok(())
 }
 
 #[test]
-fn can_download_crate_in_read_only_mode() {
-    let (app, anon, user) = TestApp::with_proxy().with_user();
+fn can_download_crate_in_read_only_mode() -> CargoResult<()> {
+    let app = App::new();
 
     app.db(|conn| {
-        CrateBuilder::new("foo_download_read_only", user.as_model().id)
+        let user = app.create_user("new_user")?;
+        CrateBuilder::new("foo_download_read_only", user.id)
             .version("1.0.0")
-            .expect_build(conn);
-        set_read_only(conn).unwrap();
-    });
+            .build(conn)
+    })?;
+    app.set_read_only()?;
 
-    anon.get::<()>("/api/v1/crates/foo_download_read_only/1.0.0/download")
-        .assert_status(302);
+    let resp = app
+        .get("/api/v1/crates/foo_download_read_only/1.0.0/download")
+        .send()?;
+    assert_eq!(302, resp.status());
 
     // We're in read only mode so the download should not have been counted
     app.db(|conn| {
@@ -44,12 +55,8 @@ fn can_download_crate_in_read_only_mode() {
 
         let dl_count = version_downloads
             .select(sum(downloads))
-            .get_result::<Option<i64>>(conn);
-        assert_eq!(Ok(None), dl_count);
+            .get_result::<Option<i64>>(conn)?;
+        assert_eq!(None, dl_count);
+        Ok(())
     })
 }
-
-fn set_read_only(conn: &PgConnection) -> QueryResult<()> {
-    diesel::sql_query("SET TRANSACTION READ ONLY").execute(conn)?;
-    Ok(())
-}