Merge pull request #7422 from Turbo87/testing-realism

Use fresh databases for all tests

Author: Turbo87

Cargo.toml

@@ -107,8 +107,3 @@ claims = "=0.7.1"
 hyper-tls = "=0.5.0"
 insta = { version = "=1.34.0", features = ["json", "redactions"] }
 tokio = "=1.34.0"
-
-[build-dependencies]
-diesel = { version = "=2.1.3", features = ["postgres"] }
-diesel_migrations = { version = "=2.1.0", features = ["postgres"] }
-dotenvy = "=0.15.7"

build.rs

@@ -92,9 +92,7 @@ impl App {
 
         let thread_pool = Arc::new(ScheduledThreadPool::new(config.db.helper_threads));
 
-        let primary_database = if config.use_test_database_pool {
-            DieselPool::new_test(&config.db, &config.db.primary.url)
-        } else {
+        let primary_database = {
             let primary_db_connection_config = ConnectionConfig {
                 statement_timeout: config.db.statement_timeout,
                 read_only: config.db.primary.read_only_mode,
@@ -119,33 +117,29 @@ impl App {
         };
 
         let replica_database = if let Some(pool_config) = config.db.replica.as_ref() {
-            if config.use_test_database_pool {
-                Some(DieselPool::new_test(&config.db, &pool_config.url))
-            } else {
-                let replica_db_connection_config = ConnectionConfig {
-                    statement_timeout: config.db.statement_timeout,
-                    read_only: true,
-                };
-
-                let replica_db_config = r2d2::Pool::builder()
-                    .max_size(pool_config.pool_size)
-                    .min_idle(pool_config.min_idle)
-                    .connection_timeout(config.db.connection_timeout)
-                    .connection_customizer(Box::new(replica_db_connection_config))
-                    .thread_pool(thread_pool);
-
-                Some(
-                    DieselPool::new(
-                        &pool_config.url,
-                        &config.db,
-                        replica_db_config,
-                        instance_metrics
-                            .database_time_to_obtain_connection
-                            .with_label_values(&["follower"]),
-                    )
-                    .unwrap(),
+            let replica_db_connection_config = ConnectionConfig {
+                statement_timeout: config.db.statement_timeout,
+                read_only: true,
+            };
+
+            let replica_db_config = r2d2::Pool::builder()
+                .max_size(pool_config.pool_size)
+                .min_idle(pool_config.min_idle)
+                .connection_timeout(config.db.connection_timeout)
+                .connection_customizer(Box::new(replica_db_connection_config))
+                .thread_pool(thread_pool);
+
+            Some(
+                DieselPool::new(
+                    &pool_config.url,
+                    &config.db,
+                    replica_db_config,
+                    instance_metrics
+                        .database_time_to_obtain_connection
+                        .with_label_values(&["follower"]),
                 )
-            }
+                .unwrap(),
+            )
         } else {
             None
         };
@@ -178,15 +172,15 @@ impl App {
 
     /// Obtain a read/write database connection from the primary pool
     #[instrument(skip_all)]
-    pub fn db_write(&self) -> Result<DieselPooledConn<'_>, PoolError> {
+    pub fn db_write(&self) -> Result<DieselPooledConn, PoolError> {
         self.primary_database.get()
     }
 
     /// Obtain a readonly database connection from the replica pool
     ///
     /// If the replica pool is disabled or unavailable, the primary pool is used instead.
     #[instrument(skip_all)]
-    pub fn db_read(&self) -> Result<DieselPooledConn<'_>, PoolError> {
+    pub fn db_read(&self) -> Result<DieselPooledConn, PoolError> {
         let read_only_pool = self.read_only_replica_database.as_ref();
         match read_only_pool.map(|pool| pool.get()) {
             // Replica is available
@@ -215,7 +209,7 @@ impl App {
     ///
     /// If the primary pool is unavailable, the replica pool is used instead, if not disabled.
     #[instrument(skip_all)]
-    pub fn db_read_prefer_primary(&self) -> Result<DieselPooledConn<'_>, PoolError> {
+    pub fn db_read_prefer_primary(&self) -> Result<DieselPooledConn, PoolError> {
         match (
             self.primary_database.get(),
             &self.read_only_replica_database,

src/app.rs

@@ -48,7 +48,6 @@ pub struct Server {
     pub downloads_persist_interval: Duration,
     pub ownership_invitations_expiration_days: u64,
     pub metrics_authorization_token: Option<String>,
-    pub use_test_database_pool: bool,
     pub instance_metrics_log_every_seconds: Option<u64>,
     pub force_unconditional_redirects: bool,
     pub blocked_routes: HashSet<String>,
@@ -209,7 +208,6 @@ impl Server {
                 .unwrap_or(Duration::from_secs(60)),
             ownership_invitations_expiration_days: 30,
             metrics_authorization_token: var("METRICS_AUTHORIZATION_TOKEN")?,
-            use_test_database_pool: false,
             instance_metrics_log_every_seconds: var_parsed("INSTANCE_METRICS_LOG_EVERY_SECONDS")?,
             force_unconditional_redirects: var("FORCE_UNCONDITIONAL_REDIRECTS")?.is_some(),
             blocked_routes: var("BLOCKED_ROUTES")?

src/config/server.rs

@@ -1,12 +1,8 @@
 use diesel::prelude::*;
-use diesel::r2d2::{self, ConnectionManager, CustomizeConnection};
+use diesel::r2d2::{self, ConnectionManager, CustomizeConnection, State};
 use prometheus::Histogram;
 use secrecy::{ExposeSecret, SecretString};
-use std::sync::{Arc, Mutex, MutexGuard};
-use std::{
-    ops::{Deref, DerefMut},
-    time::Duration,
-};
+use std::time::Duration;
 use thiserror::Error;
 use url::Url;
 
@@ -17,15 +13,9 @@ pub mod sql_types;
 pub type ConnectionPool = r2d2::Pool<ConnectionManager<PgConnection>>;
 
 #[derive(Clone)]
-pub enum DieselPool {
-    Pool {
-        pool: ConnectionPool,
-        time_to_obtain_connection_metric: Histogram,
-    },
-    BackgroundJobPool {
-        pool: ConnectionPool,
-    },
-    Test(Arc<Mutex<PgConnection>>),
+pub struct DieselPool {
+    pool: ConnectionPool,
+    time_to_obtain_connection_metric: Option<Histogram>,
 }
 
 impl DieselPool {
@@ -48,9 +38,9 @@ impl DieselPool {
         // serving errors for the first connections until the pool is initialized) and if we can't
         // establish any connection continue booting up the application. The database pool will
         // automatically be marked as unhealthy and the rest of the application will adapt.
-        let pool = DieselPool::Pool {
+        let pool = DieselPool {
             pool: r2d2_config.build_unchecked(manager),
-            time_to_obtain_connection_metric,
+            time_to_obtain_connection_metric: Some(time_to_obtain_connection_metric),
         };
         match pool.wait_until_healthy(Duration::from_secs(5)) {
             Ok(()) => {}
@@ -62,67 +52,39 @@ impl DieselPool {
     }
 
     pub fn new_background_worker(pool: r2d2::Pool<ConnectionManager<PgConnection>>) -> Self {
-        Self::BackgroundJobPool { pool }
-    }
-
-    pub(crate) fn new_test(config: &config::DatabasePools, url: &SecretString) -> DieselPool {
-        let mut conn = PgConnection::establish(&connection_url(config, url.expose_secret()))
-            .expect("failed to establish connection");
-        conn.begin_test_transaction()
-            .expect("failed to begin test transaction");
-        DieselPool::Test(Arc::new(Mutex::new(conn)))
+        Self {
+            pool,
+            time_to_obtain_connection_metric: None,
+        }
     }
 
     #[instrument(name = "db.connect", skip_all)]
-    pub fn get(&self) -> Result<DieselPooledConn<'_>, PoolError> {
-        match self {
-            DieselPool::Pool {
-                pool,
-                time_to_obtain_connection_metric,
-            } => time_to_obtain_connection_metric.observe_closure_duration(|| {
-                if let Some(conn) = pool.try_get() {
-                    Ok(DieselPooledConn::Pool(conn))
-                } else if !self.is_healthy() {
-                    Err(PoolError::UnhealthyPool)
-                } else {
-                    Ok(DieselPooledConn::Pool(pool.get()?))
-                }
-            }),
-            DieselPool::BackgroundJobPool { pool } => Ok(DieselPooledConn::Pool(pool.get()?)),
-            DieselPool::Test(conn) => Ok(DieselPooledConn::Test(
-                conn.try_lock()
-                    .map_err(|_e| PoolError::TestConnectionUnavailable)?,
-            )),
+    pub fn get(&self) -> Result<DieselPooledConn, PoolError> {
+        match self.time_to_obtain_connection_metric.as_ref() {
+            Some(time_to_obtain_connection_metric) => time_to_obtain_connection_metric
+                .observe_closure_duration(|| {
+                    if let Some(conn) = self.pool.try_get() {
+                        Ok(conn)
+                    } else if !self.is_healthy() {
+                        Err(PoolError::UnhealthyPool)
+                    } else {
+                        Ok(self.pool.get()?)
+                    }
+                }),
+            None => Ok(self.pool.get()?),
         }
     }
 
-    pub fn state(&self) -> PoolState {
-        match self {
-            DieselPool::Pool { pool, .. } | DieselPool::BackgroundJobPool { pool } => {
-                let state = pool.state();
-                PoolState {
-                    connections: state.connections,
-                    idle_connections: state.idle_connections,
-                }
-            }
-            DieselPool::Test(_) => PoolState {
-                connections: 0,
-                idle_connections: 0,
-            },
-        }
+    pub fn state(&self) -> State {
+        self.pool.state()
     }
 
     #[instrument(skip_all)]
     pub fn wait_until_healthy(&self, timeout: Duration) -> Result<(), PoolError> {
-        match self {
-            DieselPool::Pool { pool, .. } | DieselPool::BackgroundJobPool { pool } => {
-                match pool.get_timeout(timeout) {
-                    Ok(_) => Ok(()),
-                    Err(_) if !self.is_healthy() => Err(PoolError::UnhealthyPool),
-                    Err(err) => Err(PoolError::R2D2(err)),
-                }
-            }
-            DieselPool::Test(_) => Ok(()),
+        match self.pool.get_timeout(timeout) {
+            Ok(_) => Ok(()),
+            Err(_) if !self.is_healthy() => Err(PoolError::UnhealthyPool),
+            Err(err) => Err(PoolError::R2D2(err)),
         }
     }
 
@@ -131,37 +93,7 @@ impl DieselPool {
     }
 }
 
-#[derive(Debug, Copy, Clone)]
-pub struct PoolState {
-    pub connections: u32,
-    pub idle_connections: u32,
-}
-
-#[allow(clippy::large_enum_variant)]
-pub enum DieselPooledConn<'a> {
-    Pool(r2d2::PooledConnection<ConnectionManager<PgConnection>>),
-    Test(MutexGuard<'a, PgConnection>),
-}
-
-impl Deref for DieselPooledConn<'_> {
-    type Target = PgConnection;
-
-    fn deref(&self) -> &Self::Target {
-        match self {
-            DieselPooledConn::Pool(conn) => conn.deref(),
-            DieselPooledConn::Test(conn) => conn.deref(),
-        }
-    }
-}
-
-impl DerefMut for DieselPooledConn<'_> {
-    fn deref_mut(&mut self) -> &mut Self::Target {
-        match self {
-            DieselPooledConn::Pool(conn) => conn.deref_mut(),
-            DieselPooledConn::Test(conn) => conn.deref_mut(),
-        }
-    }
-}
+pub type DieselPooledConn = r2d2::PooledConnection<ConnectionManager<PgConnection>>;
 
 pub fn oneoff_connection_with_config(
     config: &config::DatabasePools,

src/db.rs

@@ -7,7 +7,6 @@ use http::StatusCode;
 #[test]
 fn can_hit_read_only_endpoints_in_read_only_mode() {
     let (_app, anon) = TestApp::init()
-        .without_test_database_pool()
         .with_config(|config| {
             config.db.primary.read_only_mode = true;
         })
@@ -20,7 +19,6 @@ fn can_hit_read_only_endpoints_in_read_only_mode() {
 #[test]
 fn cannot_hit_endpoint_which_writes_db_in_read_only_mode() {
     let (app, _, user, token) = TestApp::init()
-        .without_test_database_pool()
         .with_config(|config| {
             config.db.primary.read_only_mode = true;
         })
@@ -39,7 +37,6 @@ 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::init()
-        .without_test_database_pool()
         .with_config(|config| {
             config.db.primary.read_only_mode = true;
         })