From 2ecf2433960d2b77bf3bd46d5aa46e2223e2b14d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Fri, 24 Jan 2025 17:22:45 +0100 Subject: [PATCH 1/6] Get DB from the context if possible --- src/db.rs | 5 +++-- src/main.rs | 3 +-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/db.rs b/src/db.rs index e2d88510a..072568b23 100644 --- a/src/db.rs +++ b/src/db.rs @@ -235,8 +235,9 @@ pub async fn schedule_job( Ok(()) } -pub async fn run_scheduled_jobs(ctx: &Context, db: &DbClient) -> anyhow::Result<()> { - let jobs = get_jobs_to_execute(&db).await.unwrap(); +pub async fn run_scheduled_jobs(ctx: &Context) -> anyhow::Result<()> { + let db = &ctx.db.get().await; + let jobs = get_jobs_to_execute(&db).await?; tracing::trace!("jobs to execute: {:#?}", jobs); for job in jobs.iter() { diff --git a/src/main.rs b/src/main.rs index 362977e2c..7a1bbe57d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -401,13 +401,12 @@ fn spawn_job_runner(ctx: Arc) { loop { let ctx = ctx.clone(); let res = task::spawn(async move { - let pool = db::ClientPool::new(); let mut interval = time::interval(time::Duration::from_secs(JOB_PROCESSING_CADENCE_IN_SECS)); loop { interval.tick().await; - db::run_scheduled_jobs(&ctx, &*pool.get().await) + db::run_scheduled_jobs(&ctx) .await .context("run database scheduled jobs") .unwrap(); From 176c5c31a00a21180dea089b7c8832d4778d62c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Fri, 24 Jan 2025 17:22:58 +0100 Subject: [PATCH 2/6] Pass DB connection string to `ClientPool` --- src/db.rs | 9 +++++---- src/main.rs | 10 ++++++---- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/src/db.rs b/src/db.rs index 072568b23..5644967b0 100644 --- a/src/db.rs +++ b/src/db.rs @@ -28,6 +28,7 @@ lazy_static::lazy_static! { pub struct ClientPool { connections: Arc>>, permits: Arc, + db_url: String, } pub struct PooledClient { @@ -59,10 +60,11 @@ impl std::ops::DerefMut for PooledClient { } impl ClientPool { - pub fn new() -> ClientPool { + pub fn new(db_url: String) -> ClientPool { ClientPool { connections: Arc::new(Mutex::new(Vec::with_capacity(16))), permits: Arc::new(Semaphore::new(16)), + db_url, } } @@ -84,15 +86,14 @@ impl ClientPool { } PooledClient { - client: Some(make_client().await.unwrap()), + client: Some(make_client(&self.db_url).await.unwrap()), permit, pool: self.connections.clone(), } } } -async fn make_client() -> anyhow::Result { - let db_url = std::env::var("DATABASE_URL").expect("needs DATABASE_URL"); +async fn make_client(db_url: &str) -> anyhow::Result { if db_url.contains("rds.amazonaws.com") { let mut builder = TlsConnector::builder(); for cert in make_certificates() { diff --git a/src/main.rs b/src/main.rs index 7a1bbe57d..5fa7a737c 100644 --- a/src/main.rs +++ b/src/main.rs @@ -242,7 +242,8 @@ async fn serve_req( } async fn run_server(addr: SocketAddr) -> anyhow::Result<()> { - let pool = db::ClientPool::new(); + let db_url = std::env::var("DATABASE_URL").expect("needs DATABASE_URL"); + let pool = db::ClientPool::new(db_url.clone()); db::run_migrations(&mut *pool.get().await) .await .context("database migrations")?; @@ -271,7 +272,7 @@ async fn run_server(addr: SocketAddr) -> anyhow::Result<()> { // Run all jobs that have a schedule (recurring jobs) if !is_scheduled_jobs_disabled() { - spawn_job_scheduler(); + spawn_job_scheduler(db_url); spawn_job_runner(ctx.clone()); } @@ -361,11 +362,12 @@ async fn spawn_job_oneoffs(ctx: Arc) { /// The scheduler wakes up every `JOB_SCHEDULING_CADENCE_IN_SECS` seconds to /// check if there are any jobs ready to run. Jobs get inserted into the the /// database which acts as a queue. -fn spawn_job_scheduler() { +fn spawn_job_scheduler(db_url: String) { task::spawn(async move { loop { + let db_url = db_url.clone(); let res = task::spawn(async move { - let pool = db::ClientPool::new(); + let pool = db::ClientPool::new(db_url); let mut interval = time::interval(time::Duration::from_secs(JOB_SCHEDULING_CADENCE_IN_SECS)); From b0f628ecebd31034a4871e0adc69479ed4c1ce48 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Fri, 24 Jan 2025 18:33:45 +0100 Subject: [PATCH 3/6] Add helper code for running opt-in tests with a DB --- src/db.rs | 2 +- src/lib.rs | 3 ++ src/tests.rs | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 86 insertions(+), 1 deletion(-) create mode 100644 src/tests.rs diff --git a/src/db.rs b/src/db.rs index 5644967b0..a0cb4a940 100644 --- a/src/db.rs +++ b/src/db.rs @@ -93,7 +93,7 @@ impl ClientPool { } } -async fn make_client(db_url: &str) -> anyhow::Result { +pub async fn make_client(db_url: &str) -> anyhow::Result { if db_url.contains("rds.amazonaws.com") { let mut builder = TlsConnector::builder(); for cert in make_certificates() { diff --git a/src/lib.rs b/src/lib.rs index ce44f61ac..5b4dbb1b5 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -29,6 +29,9 @@ mod team_data; pub mod triage; pub mod zulip; +#[cfg(test)] +mod tests; + /// The name of a webhook event. #[derive(Debug)] pub enum EventName { diff --git a/src/tests.rs b/src/tests.rs new file mode 100644 index 000000000..86cd0a172 --- /dev/null +++ b/src/tests.rs @@ -0,0 +1,82 @@ +use crate::db; +use crate::db::make_client; +use std::future::Future; +use tokio_postgres::Config; + +/// Represents a connection to a Postgres database that can be +/// used in integration tests to test logic that interacts with +/// a database. +pub struct TestContext { + client: tokio_postgres::Client, + db_name: String, + original_db_url: String, + conn_handle: tokio::task::JoinHandle<()>, +} + +impl TestContext { + async fn new(db_url: &str) -> Self { + let mut config: Config = db_url.parse().expect("Cannot parse connection string"); + + // Create a new database that will be used for this specific test + let client = make_client(&db_url) + .await + .expect("Cannot connect to database"); + let db_name = format!("db{}", uuid::Uuid::new_v4().to_string().replace("-", "")); + client + .execute(&format!("CREATE DATABASE {db_name}"), &[]) + .await + .expect("Cannot create database"); + drop(client); + + // We need to connect to the database against, because Postgres doesn't allow + // changing the active database mid-connection. + config.dbname(&db_name); + let (mut client, connection) = config + .connect(tokio_postgres::NoTls) + .await + .expect("Cannot connect to the newly created database"); + let conn_handle = tokio::spawn(async move { + connection.await.unwrap(); + }); + + db::run_migrations(&mut client) + .await + .expect("Cannot run database migrations"); + Self { + client, + db_name, + original_db_url: db_url.to_string(), + conn_handle, + } + } + + async fn finish(self) { + // Cleanup the test database + // First, we need to stop using the database + drop(self.client); + self.conn_handle.await.unwrap(); + + // Then we need to connect to the default database and drop our test DB + let client = make_client(&self.original_db_url) + .await + .expect("Cannot connect to database"); + client + .execute(&format!("DROP DATABASE {}", self.db_name), &[]) + .await + .unwrap(); + } +} + +pub async fn run_test(f: F) +where + F: FnOnce(TestContext) -> Fut, + Fut: Future>, +{ + if let Ok(db_url) = std::env::var("TEST_DB_URL") { + let ctx = TestContext::new(&db_url).await; + let ctx = f(ctx).await.expect("Test failed"); + ctx.finish().await; + } else { + eprintln!("Skipping test because TEST_DB_URL was not passed"); + } +} From 6b8dd451d39208510cb4ad8706226b86bdff8e65 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Fri, 24 Jan 2025 19:22:55 +0100 Subject: [PATCH 4/6] Add DB test for finding reviewers without review preferences --- src/handlers/assign.rs | 1 + src/handlers/assign/tests/tests_db.rs | 30 +++++++++++++++++++++++++++ src/tests.rs | 11 ++++++++++ 3 files changed, 42 insertions(+) create mode 100644 src/handlers/assign/tests/tests_db.rs diff --git a/src/handlers/assign.rs b/src/handlers/assign.rs index 8ccbccd6d..add141dd5 100644 --- a/src/handlers/assign.rs +++ b/src/handlers/assign.rs @@ -39,6 +39,7 @@ use tracing as log; #[cfg(test)] mod tests { mod tests_candidates; + mod tests_db; mod tests_from_diff; } diff --git a/src/handlers/assign/tests/tests_db.rs b/src/handlers/assign/tests/tests_db.rs new file mode 100644 index 000000000..4f12f1238 --- /dev/null +++ b/src/handlers/assign/tests/tests_db.rs @@ -0,0 +1,30 @@ +#[cfg(test)] +mod tests { + use crate::handlers::assign::filter_by_capacity; + use crate::tests::run_test; + use std::collections::HashSet; + + #[tokio::test] + async fn find_reviewers_no_review_prefs() { + run_test(|ctx| async move { + ctx.add_user("usr1", 1).await; + ctx.add_user("usr2", 1).await; + let _users = + filter_by_capacity(ctx.db_client(), &candidates(&["usr1", "usr2"])).await?; + // FIXME: this test fails, because the query is wrong + // check_users(users, &["usr1", "usr2"]); + Ok(ctx) + }) + .await; + } + + fn candidates(users: &[&'static str]) -> HashSet<&'static str> { + users.into_iter().copied().collect() + } + + fn check_users(users: HashSet, expected: &[&'static str]) { + let mut users: Vec = users.into_iter().collect(); + users.sort(); + assert_eq!(users, expected); + } +} diff --git a/src/tests.rs b/src/tests.rs index 86cd0a172..a3c7e5849 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -1,5 +1,6 @@ use crate::db; use crate::db::make_client; +use crate::db::notifications::record_username; use std::future::Future; use tokio_postgres::Config; @@ -50,6 +51,16 @@ impl TestContext { } } + pub fn db_client(&self) -> &tokio_postgres::Client { + &self.client + } + + pub async fn add_user(&self, name: &str, id: u64) { + record_username(&self.client, id, name) + .await + .expect("Cannot create user"); + } + async fn finish(self) { // Cleanup the test database // First, we need to stop using the database From b10e27ca36208c8643ba3b9a5d75ffd264744189 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Fri, 24 Jan 2025 19:30:32 +0100 Subject: [PATCH 5/6] Document how to run DB tests locally --- README.md | 41 +++++++++++++++++++++++++++-------------- 1 file changed, 27 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index 814b036f0..8c34240c2 100644 --- a/README.md +++ b/README.md @@ -39,12 +39,12 @@ The general overview of what you will need to do: 3. [Configure webhook forwarding](#configure-webhook-forwarding) 4. Configure the `.env` file: - 1. Copy `.env.sample` to `.env` - 2. `GITHUB_TOKEN`: This is a token needed for Triagebot to send requests to GitHub. Go to GitHub Settings > Developer Settings > Personal Access Token, and create a new token. The `repo` permission should be sufficient. - If this is not set, Triagebot will also look in `~/.gitconfig` in the `github.oauth-token` setting. - 3. `DATABASE_URL`: This is the URL to the database. See [Configuring a database](#configuring-a-database). - 4. `GITHUB_WEBHOOK_SECRET`: Enter the secret you entered in the webhook above. - 5. `RUST_LOG`: Set this to `debug`. + 1. Copy `.env.sample` to `.env` + 2. `GITHUB_TOKEN`: This is a token needed for Triagebot to send requests to GitHub. Go to GitHub Settings > Developer Settings > Personal Access Token, and create a new token. The `repo` permission should be sufficient. + If this is not set, Triagebot will also look in `~/.gitconfig` in the `github.oauth-token` setting. + 3. `DATABASE_URL`: This is the URL to the database. See [Configuring a database](#configuring-a-database). + 4. `GITHUB_WEBHOOK_SECRET`: Enter the secret you entered in the webhook above. + 5. `RUST_LOG`: Set this to `debug`. 5. Run `cargo run --bin triagebot`. This starts the http server listening for webhooks on port 8000. 6. Add a `triagebot.toml` file to the main branch of your GitHub repo with whichever services you want to try out. @@ -109,15 +109,28 @@ You need to sign up for a free account, and also deal with configuring the GitHu 3. Configure GitHub webhooks in the test repo you created. In short: - 1. Go to the settings page for your GitHub repo. - 2. Go to the webhook section. - 3. Click "Add webhook" - 4. Include the settings: + 1. Go to the settings page for your GitHub repo. + 2. Go to the webhook section. + 3. Click "Add webhook" + 4. Include the settings: - * Payload URL: This is the URL to your Triagebot server, for example http://7e9ea9dc.ngrok.io/github-hook. This URL is displayed when you ran the `ngrok` command above. - * Content type: application/json - * Secret: Enter a shared secret (some longish random text) - * Events: "Send me everything" + * Payload URL: This is the URL to your Triagebot server, for example http://7e9ea9dc.ngrok.io/github-hook. This URL is displayed when you ran the `ngrok` command above. + * Content type: application/json + * Secret: Enter a shared secret (some longish random text) + * Events: "Send me everything" + +### Cargo tests + +You can run Cargo tests using `cargo test`. If you also want to run tests that access a Postgres database, you can specify an environment variables `TEST_DB_URL`, which should contain a connection string pointing to a running Postgres database instance: + +```bash +$ docker run --rm -it -p5432:5432 \ + -e POSTGRES_USER=triagebot \ + -e POSTGRES_PASSWORD=triagebot \ + -e POSTGRES_DB=triagebot \ + postgres:14 +$ TEST_DB_URL=postgres://triagebot:triagebot@localhost:5432/triagebot cargo test +``` ## License From 50c6d40c5619f4ac4f4e4ecb1567f30f606850ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Fri, 24 Jan 2025 19:31:06 +0100 Subject: [PATCH 6/6] Run DB tests on CI --- .github/workflows/ci.yml | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index dad0933c4..3af95ec01 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -18,6 +18,27 @@ jobs: run: rustup update stable && rustup default stable && rustup component add rustfmt - run: cargo fmt --all --check + test: + name: Test + runs-on: ubuntu-latest + env: + TEST_DB_URL: postgres://postgres:postgres@localhost:5432/postgres + services: + postgres: + image: postgres:14 + env: + POSTGRES_USER: postgres + POSTGRES_PASSWORD: postgres + POSTGRES_DB: postgres + ports: + - 5432:5432 + steps: + - uses: actions/checkout@v4 + - run: rustup toolchain install stable --profile minimal + - uses: Swatinem/rust-cache@v2 + - name: Run tests + run: cargo test --workspace --all-targets + ci: name: CI runs-on: ubuntu-latest