From 10c33b6131d75fe296134590ff27a678e77e0b98 Mon Sep 17 00:00:00 2001 From: lovasoa Date: Wed, 7 Oct 2026 00:57:41 +0200 Subject: [PATCH 1/5] test(suite) :: share helpers, close pools deterministically, slim OIDC harness Integration tests leaked every request's AppState: actix-web recycles test-built HttpRequestInner allocations into a per-request pool that the test path never drains, pinning database pools until process exit. Pooled ODBC connections keep cached prepared statements alive, which deadlocks Oracle's client at process exit. Close every test pool (clearing statement caches first, since close() alone leaves checked-out connections behind) while the test runtime is still alive, including after panics. Consolidate the suite around shared helpers (read_body_string, supports_database, request builders, OIDC login/config builders, multipart/webhook request builders) and share one AppState per test where possible, cutting duplication across oidc/uploads/requests/ data_formats/errors/server_timing/cookies/core. Net negative LOC. --- .github/workflows/ci.yml | 3 +- CONTRIBUTING.md | 8 + scripts/run-test-binaries.sh | 2 +- tests/basic/mod.rs | 16 +- tests/common/mod.rs | 119 ++++++++- tests/core/mod.rs | 152 ++++++------ tests/core/path_aliases.rs | 10 +- tests/data_formats/mod.rs | 135 +++++----- tests/errors/basic_auth.rs | 12 +- tests/errors/invalid_header.rs | 7 +- tests/errors/mod.rs | 96 ++------ tests/exec/mod.rs | 2 +- tests/oidc/mod.rs | 435 ++++++++++++++------------------- tests/parameter_binding/mod.rs | 6 +- tests/requests/mod.rs | 57 +++-- tests/requests/webhook_hmac.rs | 70 ++---- tests/server_timing/mod.rs | 73 ++---- tests/sql_test_files/mod.rs | 4 +- tests/transactions/mod.rs | 22 +- tests/uploads/mod.rs | 202 +++++++-------- 20 files changed, 696 insertions(+), 735 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index cd1535976..b3f973967 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -99,6 +99,7 @@ jobs: container: mssql db_url: "mssql://root:Password123!@127.0.0.1/sqlpage" - database: oracle + test_args: --test-threads=2 container: oracle db_url: "Driver=Oracle 21 ODBC driver;Dbq=//127.0.0.1:1521/FREEPDB1;Uid=root;Pwd=Password123!" - database: duckdb @@ -142,7 +143,7 @@ jobs: run: docker compose logs ${{ matrix.container }} - name: Run tests against ${{ matrix.database }} timeout-minutes: 5 - run: scripts/run-test-binaries.sh + run: scripts/run-test-binaries.sh ${{ matrix.test_args }} env: DATABASE_URL: ${{ matrix.db_url }} MALLOC_CHECK_: 3 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 43487637c..e5318b28b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -96,6 +96,14 @@ export DATABASE_URL=mssql://root:Password123!@localhost/sqlpage cargo test ``` +Integration tests that create application state should use `common::make_app_state_from_config` or +`common::make_app_data_from_config` and `#[actix_web::rt::test(system = "crate::common::TestSystem")]`. +The shared test runtime clears prepared statements and closes database pools before shutdown, +including after a panic. This is required because actix-web's test request utilities pin each +request's application state until process exit, which would otherwise leave native database +handles open and hang Oracle's ODBC driver at process exit. +When testing Oracle locally, use `cargo test -- --test-threads=2` to avoid overwhelming the listener. + ### End-to-End Tests We use Playwright for end-to-end testing of dynamic frontend features. diff --git a/scripts/run-test-binaries.sh b/scripts/run-test-binaries.sh index 6a02b2a77..4d0038a4d 100755 --- a/scripts/run-test-binaries.sh +++ b/scripts/run-test-binaries.sh @@ -13,6 +13,6 @@ fi for test_binary in "${test_binaries[@]}"; do echo "::group::$(basename "$test_binary")" - "$test_binary" --quiet + "$test_binary" --quiet "$@" echo "::endgroup::" done diff --git a/tests/basic/mod.rs b/tests/basic/mod.rs index 1db6cce64..bfb80c34d 100644 --- a/tests/basic/mod.rs +++ b/tests/basic/mod.rs @@ -6,18 +6,17 @@ use actix_web::{ use crate::common::req_path; -#[actix_web::test] +#[actix_web::rt::test(system = "crate::common::TestSystem")] async fn test_index_ok() { let resp = req_path("/").await.unwrap(); assert_eq!(resp.status(), http::StatusCode::OK); - let body = test::read_body(resp).await; - assert!(body.starts_with(b"")); - let body = String::from_utf8(body.to_vec()).unwrap(); + let body = crate::common::read_body_string(resp).await; + assert!(body.starts_with("")); assert!(body.contains("It works !")); assert!(!body.contains("error")); } -#[actix_web::test] +#[actix_web::rt::test(system = "crate::common::TestSystem")] async fn test_access_config_forbidden() { let resp_result = req_path("/sqlpage/sqlpage.json").await; assert!( @@ -33,7 +32,7 @@ async fn test_access_config_forbidden() { ); } -#[actix_web::test] +#[actix_web::rt::test(system = "crate::common::TestSystem")] async fn test_static_files() { let resp = req_path("/tests/it_works.txt").await.unwrap(); assert_eq!(resp.status(), http::StatusCode::OK); @@ -41,13 +40,12 @@ async fn test_static_files() { assert_eq!(&body, &b"It works !"[..]); } -#[actix_web::test] +#[actix_web::rt::test(system = "crate::common::TestSystem")] async fn test_spaces_in_file_names() { let resp = req_path("/tests/core/spaces%20in%20file%20name.sql") .await .unwrap(); assert_eq!(resp.status(), http::StatusCode::OK); - let body = test::read_body(resp).await; - let body_str = String::from_utf8(body.to_vec()).unwrap(); + let body_str = crate::common::read_body_string(resp).await; assert!(body_str.contains("It works !"), "{body_str}"); } diff --git a/tests/common/mod.rs b/tests/common/mod.rs index da2b689f0..93d7bfb5b 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -38,16 +38,127 @@ pub(crate) async fn get_request_to(path: &str) -> actix_web::Result } pub(crate) async fn make_app_data_from_config(config: AppConfig) -> Data { - let state = AppState::init(&config).await.unwrap(); + let state = make_app_state_from_config(&config).await.unwrap(); Data::new(state) } +// Pools must be emptied while the test runtime is still running. +// +// Each request built with `TestRequest::to_srv_request()` leaks its +// `Data`: actix-web recycles the `HttpRequestInner` allocation into +// a per-request pool on drop, and that pool is only drained by a real server, +// never on the test path. The leaked `AppState` pins its database pool (and +// its pooled connections) until process exit. The ODBC backend prepares and +// caches a native statement for every distinct parameterized query, and +// Oracle's ODBC driver deadlocks in its process destructor when connections +// still hold native statements at exit. +// +// Note that `pool.close()` alone is not sufficient: it only closes idle +// connections, so one still checked out (e.g. by a return-to-pool task +// finishing the test's last request) would keep its cached statements alive. +// Acquire every connection and clear its cache first to free those handles. +thread_local! { + static TEST_POOLS: std::cell::RefCell> = const { std::cell::RefCell::new(Vec::new()) }; +} + +pub(crate) struct TestSystem(actix_web::rt::SystemRunner); + +impl TestSystem { + pub(crate) fn new() -> Self { + Self(actix_web::rt::System::new()) + } + + pub(crate) fn block_on(&self, future: F) -> F::Output { + use futures_util::FutureExt as _; + + self.0.block_on(async { + let result = std::panic::AssertUnwindSafe(future).catch_unwind().await; + // Empty every pool created by the test while the runtime is still + // alive, even if the test panicked. Clearing first releases cached + // prepared statements on connections that may still be checked out + // (which `close()` alone would leave behind); closing then + // disconnects each pooled connection. The pool objects themselves + // may stay alive (see above), but they are left empty. + let pools = TEST_POOLS.with(std::cell::RefCell::take); + for pool in pools { + use sqlx::connection::Connection as _; + + let mut connections = Vec::new(); + for _ in 0..pool.size() { + let mut connection = pool.acquire().await.unwrap(); + connection.clear_cached_statements().await.unwrap(); + connections.push(connection); + } + drop(connections); + pool.close().await; + } + match result { + Ok(output) => output, + Err(panic) => std::panic::resume_unwind(panic), + } + }) + } +} + +pub(crate) async fn make_app_state_from_config(config: &AppConfig) -> anyhow::Result { + let state = AppState::init(config).await?; + TEST_POOLS.with(|pools| pools.borrow_mut().push(state.db.connection.clone())); + Ok(state) +} + +#[test] +fn test_system_closes_pools_on_success_and_panic() { + for panic_in_test in [false, true] { + let system = TestSystem::new(); + let mut pool = None; + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + system.block_on(async { + let mut config = test_config(); + config.database_url = "sqlite::memory:".to_owned(); + let state = make_app_state_from_config(&config).await.unwrap(); + pool = Some(state.db.connection.clone()); + assert!(!panic_in_test, "intentional test panic"); + }); + })); + assert_eq!(result.is_err(), panic_in_test); + assert!(pool.unwrap().is_closed()); + } +} + +/// Reads a whole response body as a UTF-8 string, failing the test otherwise. +pub(crate) async fn read_body_string(resp: actix_web::dev::ServiceResponse) -> String +where + B: actix_web::body::MessageBody, +{ + String::from_utf8(actix_web::test::read_body(resp).await.to_vec()).unwrap() +} + +/// Whether the database engine is one of `kinds`. +/// Tests that only run on some engines return early otherwise. +pub(crate) fn supports_database( + db: &sqlpage::webserver::database::Database, + kinds: &[sqlpage::webserver::database::SupportedDatabase], +) -> bool { + kinds.contains(&db.info.database_type) +} + pub(crate) async fn make_app_data() -> Data { init_log(); let config = test_config(); make_app_data_from_config(config).await } +/// Creates test application state running in the given environment +/// (development enables debug helpers like `Server-Timing`). +pub(crate) async fn make_app_data_with_env( + environment: sqlpage::app_config::DevOrProd, +) -> Data { + init_log(); + let mut config = test_config(); + config.environment = environment; + make_app_data_from_config(config).await +} + pub(crate) async fn req_path( path: impl AsRef, ) -> Result { @@ -76,9 +187,9 @@ async fn req_path_with_app_data_and_accept( accept: header::Accept, ) -> anyhow::Result { let path = path.as_ref(); - let req = TestRequest::get() - .uri(path) - .app_data(app_data) + let req = get_request_to_with_data(path, app_data) + .await + .map_err(|e| anyhow::anyhow!("Failed to build request for {path}: {e}"))? .insert_header(("cookie", "test_cook=123")) .insert_header(("authorization", "Basic dGVzdDp0ZXN0")) .insert_header(accept) diff --git a/tests/core/mod.rs b/tests/core/mod.rs index 6062d877c..036429e88 100644 --- a/tests/core/mod.rs +++ b/tests/core/mod.rs @@ -3,13 +3,48 @@ use sqlpage::{ AppState, webserver::{self, make_placeholder}, }; -use sqlx::executor::Executor as _; use crate::common::{make_app_data_from_config, req_path, req_path_with_app_data, test_config}; mod path_aliases; -#[actix_web::test] +/// Creates the `sqlpage_files` table if needed and stores `contents` at `path`. +/// Other tests share this database, so the table is never dropped. +async fn store_file_in_db(state: &AppState, path: &str, contents: &[u8]) { + use sqlx::executor::Executor as _; + + let create_table_sql = + sqlpage::filesystem::DbFsQueries::get_create_table_sql(state.db.info.database_type); + if state + .db + .connection + .execute("SELECT 1 FROM sqlpage_files WHERE 1 = 0") + .await + .is_err() + { + state.db.connection.execute(create_table_sql).await.unwrap(); + } + let delete_sql = format!("DELETE FROM sqlpage_files WHERE path = '{path}'"); + state + .db + .connection + .execute(delete_sql.as_str()) + .await + .unwrap(); + let insert_sql = format!( + "INSERT INTO sqlpage_files(path, contents) VALUES ({}, {})", + make_placeholder(state.db.info.kind, 1), + make_placeholder(state.db.info.kind, 2) + ); + sqlx::query::query(&insert_sql) + .bind(path) + .bind(contents) + .execute(&state.db.connection) + .await + .unwrap(); +} + +#[actix_web::rt::test(system = "crate::common::TestSystem")] async fn test_concurrent_requests() { let components = [ "table", "form", "card", "datagrid", "hero", "list", "timeline", @@ -28,12 +63,8 @@ async fn test_concurrent_requests() { for result in results { let resp = result.unwrap(); assert_eq!(resp.status(), StatusCode::OK); - let body = test::read_body(resp).await; - assert!( - body.starts_with(b""), - "Expected html doctype" - ); - let body = String::from_utf8(body.to_vec()).unwrap(); + let body = crate::common::read_body_string(resp).await; + assert!(body.starts_with(""), "Expected html doctype"); assert!( body.contains("It works !"), "Expected to contain: It works !, but got: {body}" @@ -42,13 +73,13 @@ async fn test_concurrent_requests() { } } -#[actix_web::test] +#[actix_web::rt::test(system = "crate::common::TestSystem")] async fn test_datagrid_description_presence_controls_placeholder() { let resp = req_path("/tests/components/datagrid_icon_only.sql") .await .unwrap(); assert_eq!(resp.status(), StatusCode::OK); - let body = String::from_utf8(test::read_body(resp).await.to_vec()).unwrap(); + let body = crate::common::read_body_string(resp).await; assert!(body.contains("Facebook"), "{body}"); assert!(body.contains("Empty"), "{body}"); assert!(body.contains("Missing"), "{body}"); @@ -56,7 +87,7 @@ async fn test_datagrid_description_presence_controls_placeholder() { assert_eq!(body.matches('–').count(), 1, "{body}"); } -#[actix_web::test] +#[actix_web::rt::test(system = "crate::common::TestSystem")] async fn test_routing_with_db_fs() { let mut config = test_config(); if config.database_url.contains("memory") { @@ -64,52 +95,34 @@ async fn test_routing_with_db_fs() { } config.site_prefix = "/prefix/".to_string(); - let state = AppState::init(&config).await.unwrap(); + let state = crate::common::make_app_state_from_config(&config) + .await + .unwrap(); - if matches!( - state.db.info.database_type, - webserver::database::SupportedDatabase::Oracle + if crate::common::supports_database( + &state.db, + &[webserver::database::SupportedDatabase::Oracle], ) { return; } - let create_table_sql = - sqlpage::filesystem::DbFsQueries::get_create_table_sql(state.db.info.database_type); - // Other tests share this database, so never drop a table their initialized state may use. - if state - .db - .connection - .execute("SELECT 1 FROM sqlpage_files WHERE 1 = 0") - .await - .is_err() - { - state.db.connection.execute(create_table_sql).await.unwrap(); - } - state - .db - .connection - .execute("DELETE FROM sqlpage_files WHERE path = 'on_db.sql'") - .await - .unwrap(); - let insert_sql = format!( - "INSERT INTO sqlpage_files(path, contents) VALUES ('on_db.sql', {})", - make_placeholder(state.db.info.kind, 1) - ); - sqlx::query::query(&insert_sql) - .bind("select ''text'' as component, ''Hi from db !'' AS contents;".as_bytes()) - .execute(&state.db.connection) + store_file_in_db( + &state, + "on_db.sql", + b"select ''text'' as component, ''Hi from db !'' AS contents;", + ) + .await; + + let state = crate::common::make_app_state_from_config(&config) .await .unwrap(); - - let state = AppState::init(&config).await.unwrap(); let app_data = actix_web::web::Data::new(state); let resp = req_path_with_app_data("/prefix/on_db.sql", app_data.clone()) .await .unwrap(); assert_eq!(resp.status(), StatusCode::OK); - let body = test::read_body(resp).await; - let body_str = String::from_utf8(body.to_vec()).unwrap(); + let body_str = crate::common::read_body_string(resp).await; assert!( body_str.contains("Hi from db !"), "{body_str}\nexpected to contain: Hi from db !" @@ -117,7 +130,7 @@ async fn test_routing_with_db_fs() { } #[cfg(unix)] -#[actix_web::test] +#[actix_web::rt::test(system = "crate::common::TestSystem")] async fn test_non_unicode_static_path_returns_bad_request_with_db_fs() { let mut config = test_config(); if !config.database_url.starts_with("sqlite") { @@ -126,30 +139,15 @@ async fn test_non_unicode_static_path_returns_bad_request_with_db_fs() { config.database_url = "sqlite://file:test_non_unicode_static_path?mode=memory&cache=shared".to_string(); - let state = AppState::init(&config).await.unwrap(); - let expected_db_path = "\u{FFFD}.txt"; - let mut conn = state.db.connection.acquire().await.unwrap(); - - (&mut *conn) - .execute(sqlpage::filesystem::DbFsQueries::get_create_table_sql( - webserver::database::SupportedDatabase::Sqlite, - )) + let state = crate::common::make_app_state_from_config(&config) .await .unwrap(); - let insert_sql = format!( - "INSERT INTO sqlpage_files(path, contents) VALUES ({}, {})", - make_placeholder(state.db.info.kind, 1), - make_placeholder(state.db.info.kind, 2) - ); - sqlx::query::query(&insert_sql) - .bind(expected_db_path) - .bind("file from db fs".as_bytes()) - .execute(&mut *conn) + let expected_db_path = "\u{FFFD}.txt"; + store_file_in_db(&state, expected_db_path, b"file from db fs").await; + + let state = crate::common::make_app_state_from_config(&config) .await .unwrap(); - drop(conn); - - let state = AppState::init(&config).await.unwrap(); let app_data = actix_web::web::Data::new(state); let req = test::TestRequest::get() .uri("/%FF.txt") @@ -165,11 +163,13 @@ async fn test_non_unicode_static_path_returns_bad_request_with_db_fs() { ); } -#[actix_web::test] +#[actix_web::rt::test(system = "crate::common::TestSystem")] async fn test_routing_with_prefix() { let mut config = test_config(); config.site_prefix = "/prefix/".to_string(); - let state = AppState::init(&config).await.unwrap(); + let state = crate::common::make_app_state_from_config(&config) + .await + .unwrap(); let app_data = actix_web::web::Data::new(state); let resp = req_path_with_app_data( @@ -179,8 +179,7 @@ async fn test_routing_with_prefix() { .await .unwrap(); assert_eq!(resp.status(), StatusCode::OK); - let body = test::read_body(resp).await; - let body_str = String::from_utf8(body.to_vec()).unwrap(); + let body_str = crate::common::read_body_string(resp).await; assert!( body_str.contains("It works !"), "{body_str}\nexpected to contain: It works !" @@ -193,8 +192,7 @@ async fn test_routing_with_prefix() { let resp = req_path_with_app_data("/prefix/nonexistent.sql", app_data.clone()) .await .expect("should handle 404"); - let body = test::read_body(resp).await; - let body_str = String::from_utf8(body.to_vec()).unwrap(); + let body_str = crate::common::read_body_string(resp).await; assert!( body_str.contains("404"), "Response should contain \"404\", but got:\n{body_str}" @@ -220,7 +218,7 @@ async fn test_routing_with_prefix() { assert_eq!(location.to_str().unwrap(), "/prefix/"); } -#[actix_web::test] +#[actix_web::rt::test(system = "crate::common::TestSystem")] async fn test_hidden_files() { let resp_result = req_path("/tests/core/.hidden.sql").await; assert!( @@ -238,7 +236,7 @@ async fn test_hidden_files() { ); } -#[actix_web::test] +#[actix_web::rt::test(system = "crate::common::TestSystem")] async fn test_official_website_documentation() { let app_data = make_app_data_for_official_website().await; let resp = req_path_with_app_data("/component.sql?component=button", app_data) @@ -247,15 +245,14 @@ async fn test_official_website_documentation() { panic!("Failed to get response for /component.sql?component=button: {e}") }); assert_eq!(resp.status(), StatusCode::OK); - let body = test::read_body(resp).await; - let body_str = String::from_utf8(body.to_vec()).unwrap(); + let body_str = crate::common::read_body_string(resp).await; assert!( body_str.contains(r#"