From 5910387eb74ac00fc89caaae0f97192c3c2e20ec Mon Sep 17 00:00:00 2001 From: le king fu Date: Tue, 6 Oct 2026 21:46:26 -0400 Subject: [PATCH] test(profiles): pin the review's quick suggestions on the catch-up (#390) From the /pr-review of PR #394: - the profile database opens only once prepare_profile_db has resolved (deferred preparation; a load started early used to pass, RT1); - a real VACUUM INTO failure, through a dangling symlink, names the full path and comes out scrubbed (RV6); the test first checks the raw message does name the folder; - deleting a profile removes the -journal side file, the only one this app produces (RV5); - test folders are removed when each test ends, a failing one included (TestDir guard), instead of 42 folders left in /tmp per run. Co-Authored-By: Claude Opus 5.5 --- src-tauri/src/commands/profile_commands.rs | 7 +- src-tauri/src/database/profile_db.rs | 89 ++++++++++++++++++---- src/services/db.test.ts | 22 ++++++ 3 files changed, 102 insertions(+), 16 deletions(-) diff --git a/src-tauri/src/commands/profile_commands.rs b/src-tauri/src/commands/profile_commands.rs index 49f99a4..5872da0 100644 --- a/src-tauri/src/commands/profile_commands.rs +++ b/src-tauri/src/commands/profile_commands.rs @@ -496,12 +496,14 @@ mod tests { #[test] fn deleting_a_profile_removes_its_files_and_only_its_files() { - let dir = std::env::temp_dir().join(format!("simpl-resultat-delete-{}", std::process::id())); - let _ = fs::remove_dir_all(&dir); + let dir = profile_db::TestDir::new("delete"); let backups = dir.join(profile_db::MIGRATION_BACKUP_DIR); fs::create_dir_all(&backups).unwrap(); for name in [ "profile_0a1b2c3d.db", + // The only side file this app produces: sqlx leaves SQLite's + // rollback journal by default, `-wal`/`-shm` come with WAL. + "profile_0a1b2c3d.db-journal", "profile_0a1b2c3d.db-wal", "profile_0a1b2c3d.db-shm", "profile_99999999.db", @@ -528,6 +530,5 @@ mod tests { "profile_99999999.db".to_string(), ] ); - let _ = fs::remove_dir_all(&dir); } } diff --git a/src-tauri/src/database/profile_db.rs b/src-tauri/src/database/profile_db.rs index 199eb83..25bbaf4 100644 --- a/src-tauri/src/database/profile_db.rs +++ b/src-tauri/src/database/profile_db.rs @@ -556,12 +556,56 @@ pub fn prepare_at(db_path: &Path, db_filename: &str, migrations: &[Migration]) - Ok(report) } +/// A folder under the system temp dir for one test, removed when the guard +/// drops, a failing test included. Cargo runs tests in parallel, so each gets +/// its own name. +#[cfg(test)] +pub(crate) struct TestDir(std::path::PathBuf); + +#[cfg(test)] +impl TestDir { + pub(crate) fn new(tag: &str) -> Self { + use std::sync::atomic::{AtomicUsize, Ordering}; + static COUNTER: AtomicUsize = AtomicUsize::new(0); + let dir = std::env::temp_dir().join(format!( + "simpl-resultat-{}-{}-{}", + tag, + std::process::id(), + COUNTER.fetch_add(1, Ordering::SeqCst) + )); + let _ = fs::remove_dir_all(&dir); + fs::create_dir_all(&dir).unwrap(); + TestDir(dir) + } +} + +#[cfg(test)] +impl std::ops::Deref for TestDir { + type Target = Path; + fn deref(&self) -> &Path { + &self.0 + } +} + +#[cfg(test)] +impl AsRef for TestDir { + fn as_ref(&self) -> &Path { + &self.0 + } +} + +#[cfg(test)] +impl Drop for TestDir { + fn drop(&mut self) { + let _ = fs::remove_dir_all(&self.0); + } +} + #[cfg(test)] mod tests { use super::*; use std::collections::BTreeMap; use std::path::PathBuf; - use std::sync::atomic::{AtomicUsize, Ordering}; // The SQL each release ran to create a secondary profile // (`get_new_profile_init_sql` of that tag), copied verbatim from git. @@ -604,18 +648,9 @@ mod tests { (from..=to).collect() } - /// Cargo runs tests in parallel: every test gets its own folder. - fn unique_dir(tag: &str) -> PathBuf { - static COUNTER: AtomicUsize = AtomicUsize::new(0); - let dir = std::env::temp_dir().join(format!( - "simpl-resultat-profile-db-{}-{}-{}", - std::process::id(), - tag, - COUNTER.fetch_add(1, Ordering::SeqCst) - )); - let _ = fs::remove_dir_all(&dir); - fs::create_dir_all(&dir).unwrap(); - dir + /// Every test gets its own folder, removed when the test ends. + fn unique_dir(tag: &str) -> TestDir { + TestDir::new(&format!("profile-db-{}", tag)) } fn create_db(dir: &Path, name: &str, init: &[&str]) -> PathBuf { @@ -1288,6 +1323,34 @@ mod tests { assert!(!column_exists(&conn, "import_sources", "header_signature").unwrap()); } + #[cfg(unix)] + #[test] + fn a_real_backup_failure_names_no_folder() { + // `VACUUM INTO` through a dangling symlink fails even as root (a + // read-only folder would not: CI runs as root), and SQLite's message + // names the full path. Only `scrub` keeps it out of the report. + let dir = unique_dir("leak"); + let path = create_db(&dir, SECONDARY, &[CONSOLIDATED_V0_10_0]); + let backups = dir.join(MIGRATION_BACKUP_DIR); + fs::create_dir_all(&backups).unwrap(); + let tmp = backups.join("profile_0a1b2c3d-before-v17.db.tmp"); + std::os::unix::fs::symlink(dir.join("missing").join("target.db"), &tmp).unwrap(); + let folder = dir.to_str().unwrap().to_string(); + + // The probe proves something only if the raw message names the folder. + let raw = backup_to(&open(&path), &backups, "profile_0a1b2c3d", 17).unwrap_err(); + assert!(raw.starts_with("backup_failed"), "{}", raw); + assert!(raw.contains(&folder), "{}", raw); + + let report = prepare_at(&path, SECONDARY, &migrations()).unwrap(); + + let error = report.catch_up_error.unwrap(); + assert!(error.starts_with("backup_failed"), "{}", error); + assert!(!error.contains(&folder), "{}", error); + assert!(error.contains(""), "{}", error); + assert!(!has_table(&open(&path), "_sqlx_migrations")); + } + #[test] fn remove_backups_takes_only_this_profile() { let dir = unique_dir("cleanup"); diff --git a/src/services/db.test.ts b/src/services/db.test.ts index 9672428..2f7d5d1 100644 --- a/src/services/db.test.ts +++ b/src/services/db.test.ts @@ -178,6 +178,28 @@ describe("a profile is prepared before the plugin opens it (#390)", () => { ); }); + it("opens the database only once the preparation has finished", async () => { + // In production the command runs on the main thread while Database.load + // is asynchronous: started early, the two would overlap on the same file. + let finish: (report: ProfilePreparation | null) => void = () => {}; + h.prepare = () => + new Promise((resolve) => { + finish = resolve; + }); + + const connecting = connectToProfile("profile_0a1b2c3d.db"); + await vi.waitFor(() => + expect(invoke).toHaveBeenCalledWith("prepare_profile_db", { dbFilename: "profile_0a1b2c3d.db" }) + ); + // Give a load started too early every chance to show up. + await new Promise((resolve) => setTimeout(resolve, 10)); + expect(Database.load).not.toHaveBeenCalled(); + + finish(null); + await connecting; + expect(Database.load).toHaveBeenCalledWith("sqlite:profile_0a1b2c3d.db"); + }); + it("still opens the profile when the preparation fails, and says so in the Journal", async () => { h.prepare = async () => { throw "invalid_profile_db_filename";