test(profiles): pin the review's quick suggestions on the catch-up (#390)
All checks were successful
PR Check — Frontend / frontend (pull_request) Successful in 1m49s
PR Check — Rust / rust (pull_request) Successful in 3m39s

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 <noreply@anthropic.com>
This commit is contained in:
le king fu 2026-10-06 21:46:26 -04:00
parent 71147c5083
commit 5910387eb7
3 changed files with 102 additions and 16 deletions

View file

@ -496,12 +496,14 @@ mod tests {
#[test] #[test]
fn deleting_a_profile_removes_its_files_and_only_its_files() { 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 dir = profile_db::TestDir::new("delete");
let _ = fs::remove_dir_all(&dir);
let backups = dir.join(profile_db::MIGRATION_BACKUP_DIR); let backups = dir.join(profile_db::MIGRATION_BACKUP_DIR);
fs::create_dir_all(&backups).unwrap(); fs::create_dir_all(&backups).unwrap();
for name in [ for name in [
"profile_0a1b2c3d.db", "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-wal",
"profile_0a1b2c3d.db-shm", "profile_0a1b2c3d.db-shm",
"profile_99999999.db", "profile_99999999.db",
@ -528,6 +530,5 @@ mod tests {
"profile_99999999.db".to_string(), "profile_99999999.db".to_string(),
] ]
); );
let _ = fs::remove_dir_all(&dir);
} }
} }

View file

@ -556,12 +556,56 @@ pub fn prepare_at(db_path: &Path, db_filename: &str, migrations: &[Migration]) -
Ok(report) 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<Path> 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)] #[cfg(test)]
mod tests { mod tests {
use super::*; use super::*;
use std::collections::BTreeMap; use std::collections::BTreeMap;
use std::path::PathBuf; use std::path::PathBuf;
use std::sync::atomic::{AtomicUsize, Ordering};
// The SQL each release ran to create a secondary profile // The SQL each release ran to create a secondary profile
// (`get_new_profile_init_sql` of that tag), copied verbatim from git. // (`get_new_profile_init_sql` of that tag), copied verbatim from git.
@ -604,18 +648,9 @@ mod tests {
(from..=to).collect() (from..=to).collect()
} }
/// Cargo runs tests in parallel: every test gets its own folder. /// Every test gets its own folder, removed when the test ends.
fn unique_dir(tag: &str) -> PathBuf { fn unique_dir(tag: &str) -> TestDir {
static COUNTER: AtomicUsize = AtomicUsize::new(0); TestDir::new(&format!("profile-db-{}", tag))
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
} }
fn create_db(dir: &Path, name: &str, init: &[&str]) -> PathBuf { 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()); 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("<profile folder>"), "{}", error);
assert!(!has_table(&open(&path), "_sqlx_migrations"));
}
#[test] #[test]
fn remove_backups_takes_only_this_profile() { fn remove_backups_takes_only_this_profile() {
let dir = unique_dir("cleanup"); let dir = unique_dir("cleanup");

View file

@ -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 () => { it("still opens the profile when the preparation fails, and says so in the Journal", async () => {
h.prepare = async () => { h.prepare = async () => {
throw "invalid_profile_db_filename"; throw "invalid_profile_db_filename";