From b5e30547746e56169a86504109e164ede4c1c979 Mon Sep 17 00:00:00 2001 From: aviezerl Date: Mon, 3 Aug 2026 17:48:42 +0300 Subject: [PATCH 1/2] chore: make the parallel test suite fail loudly and recover its root Three test-harness fixes, all found while diagnosing PR #151. 1. .Rbuildignore the two hidden files under tests/testthat/fixtures/tiny-hub-groot (.db.cache, .ro_attributes). They produced a "checking for hidden files and directories" NOTE on every CRAN check. Their only consumer is skip_on_cran()-gated, and .Rbuildignore affects the built tarball only, not devtools::test. 2. create_isolated_test_db() now checks the exit status of its cp -r / ln -s calls and of the chrom_sizes.txt copy. Previously a failure (a full tempdir() being the usual cause) left a half-built DB and surfaced much later as e.g. "Interval test.fixedbin does not exist" in an unrelated file, which is exactly how it wasted time on #151. 3. Its teardown no longer unsets GROOT when the previous root has already been removed. That happens whenever the preceding file in a parallel worker was itself isolated: the worker was left with no database, and the next file relying on the ambient root failed with "Database directory does not exist" or "Chromosome chr1 does not exist ... Known chromosomes: chrA". It now falls back to the shared test db, which covers the ~22 files that read main db fixtures without isolating. Also adds create_isolated_test_db() to test-ggenome.R and test-gintervals-from-strings.R (the two that failed this way), and documents the TMPDIR requirement for parallel runs in CONTRIBUTING.md. This does not make the suite fully deterministic. Files that manage their own temp DBs can still leave GROOT dangling after withr deletes the directory - test-gtrack-create-meta-indexed.R's setup_db() does exactly that and still fails intermittently. That is a separate cleanup. Claude-Session: https://claude.ai/code/session_01GCdUQC4k8tf93iHnZHSTVS --- .Rbuildignore | 2 + CONTRIBUTING.md | 14 +++++++ tests/testthat/helper-test_db.R | 39 +++++++++++++++---- tests/testthat/test-ggenome.R | 2 + tests/testthat/test-gintervals-from-strings.R | 2 + 5 files changed, 51 insertions(+), 8 deletions(-) diff --git a/.Rbuildignore b/.Rbuildignore index 5b4721942..46bb94927 100644 --- a/.Rbuildignore +++ b/.Rbuildignore @@ -33,3 +33,5 @@ ^.a5c$ ^README\.html$ ^tools$ +^tests/testthat/fixtures/tiny-hub-groot/\.db\.cache$ +^tests/testthat/fixtures/tiny-hub-groot/\.ro_attributes$ diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e3553a595..2f127618a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -40,6 +40,20 @@ Sys.setenv(TESTTHAT_PARALLEL = "TRUE") devtools::test() ``` +A parallel run gives each test file its own copy of the test database under +`tempdir()`, so point `TMPDIR` at a filesystem with several GB free before +starting R: + +```sh +TMPDIR=/path/with/space R -e 'Sys.setenv(TESTTHAT_PARALLEL="TRUE"); devtools::test()' +``` + +On a shared machine `/tmp` often sits on a full root filesystem. Running out +of space mid-run leaves partially copied test databases, and the resulting +failures point at whichever test file happened to use them rather than at the +disk — expect errors like `Interval test.fixedbin does not exist`. Failures +that move between files across runs are a symptom of this, not of flaky tests. + GitHub Actions runs `R CMD check` on every push and PR — treat that as the authoritative signal. diff --git a/tests/testthat/helper-test_db.R b/tests/testthat/helper-test_db.R index 436fb7d6a..ada71bba4 100644 --- a/tests/testthat/helper-test_db.R +++ b/tests/testthat/helper-test_db.R @@ -107,22 +107,37 @@ create_isolated_test_db <- function() { "/net/mraid20/export/tgdata/db/tgdb/misha_test_db/" } + # Fail loudly instead of leaving a half-built DB behind. A silent failure + # here (a full tempdir() is the usual cause) surfaces much later as a + # baffling "Interval test.fixedbin does not exist" in an unrelated test. + checked_system <- function(cmd) { + status <- system(cmd) + if (status != 0) { + stop(sprintf( + "create_isolated_test_db(): `%s` failed with status %d.\nIs tempdir() (%s) out of space? Set TMPDIR to a volume with room.", + cmd, status, tempdir() + ), call. = FALSE) + } + } + # Create unique temp dir for this test file/process testdb_dir <- tempfile(pattern = "misha_testdb_", tmpdir = tempdir()) dir.create(testdb_dir, showWarnings = FALSE) # Copy small files that might be read - file.copy( - file.path(source_db, "chrom_sizes.txt"), - testdb_dir - ) + if (!file.copy(file.path(source_db, "chrom_sizes.txt"), testdb_dir)) { + stop(sprintf( + "create_isolated_test_db(): failed to copy chrom_sizes.txt into %s (out of space?)", + testdb_dir + ), call. = FALSE) + } # Copy interval sets and PSSM directories - system(sprintf("cp -r %s/intervs %s/", source_db, testdb_dir)) - system(sprintf("cp -r %s/pssms %s/", source_db, testdb_dir)) + checked_system(sprintf("cp -r %s/intervs %s/", source_db, testdb_dir)) + checked_system(sprintf("cp -r %s/pssms %s/", source_db, testdb_dir)) # Symlink the large seq directory (read-only) - system(sprintf("ln -s %s/seq %s/seq", source_db, testdb_dir)) + checked_system(sprintf("ln -s %s/seq %s/seq", source_db, testdb_dir)) # Create tracks directory for this test file tracks_dir <- file.path(testdb_dir, "tracks") @@ -137,7 +152,7 @@ create_isolated_test_db <- function() { # Symlink each fixture track for (track in source_tracks) { - system(sprintf( + checked_system(sprintf( "ln -s %s/tracks/%s %s/tracks/%s", source_db, track, testdb_dir, track )) @@ -170,6 +185,14 @@ create_isolated_test_db <- function() { if (!is.null(current_groot) && identical(current_groot, testdb_dir)) { if (!is.null(prev_groot) && dir.exists(prev_groot)) { suppressMessages(gdb.init(prev_groot)) + } else if (dir.exists(source_db)) { + # prev_groot was itself an isolated db that has already been + # torn down. Fall back to the shared test db rather than + # unsetting GROOT: in a parallel run the next file in this + # worker may be one that relies on the ambient root, and it + # would otherwise fail with a confusing "does not exist" or + # "Chromosome chr1 does not exist" error. + suppressMessages(gdb.init(source_db)) } else { rm("GROOT", envir = .misha) } diff --git a/tests/testthat/test-ggenome.R b/tests/testthat/test-ggenome.R index b49e1676c..52121f4c5 100644 --- a/tests/testthat/test-ggenome.R +++ b/tests/testthat/test-ggenome.R @@ -1,3 +1,5 @@ +create_isolated_test_db() + # Local test helper: read a FASTA file into a named list of sequences read_fasta <- function(fasta_path) { lines <- readLines(fasta_path) diff --git a/tests/testthat/test-gintervals-from-strings.R b/tests/testthat/test-gintervals-from-strings.R index 584b1c3f4..16fe36659 100644 --- a/tests/testthat/test-gintervals-from-strings.R +++ b/tests/testthat/test-gintervals-from-strings.R @@ -1,3 +1,5 @@ +create_isolated_test_db() + test_that("gintervals.from_strings parses a single chrom:start-end string", { res <- gintervals.from_strings("chr1:100-200") expect_equal(nrow(res), 1) From 595fdc38f161e459df1fcea31b1353494c9d861a Mon Sep 17 00:00:00 2001 From: aviezerl Date: Mon, 3 Aug 2026 18:34:46 +0300 Subject: [PATCH 2/2] chore: stop test files leaving a dangling GROOT behind Follow-up to the previous commit, which fixed create_isolated_test_db()'s teardown but left the other half of the problem: files that build their own temporary database and re-root into it. When withr removes that directory, .misha$GROOT still points at it, and under TESTTHAT_PARALLEL the next file in the same worker inherits the dangling root. It then fails on something unrelated to its own subject - "Database directory does not exist", "Chromosome chr1 does not exist ... Known chromosomes: chrA", or "Cannot delete track from read-only database" (a removed directory is not writable, so file.access reports it as read-only). 27 files re-root without restoring, but only the 10 that delete the directory mid-run can strand a later file. Adds ensure_valid_groot() / restore_groot_on_exit() to helper-test_db.R and calls them there. For helper-liftover.R the restore goes in its existing teardown_env() defer, which covers every file that uses it. Also .Rbuildignore ^\.worktrees$. Local git worktrees live inside the repo, so R CMD build was sweeping all 15 of them into the tarball: 3.1 GB instead of 1.8 MB, which made local R CMD build/check unusable. Verified the built tarball now contains no hidden files at all and no worktrees. Suite run three consecutive times: FAIL 0 | PASS 19271 each time. Before these two commits the same code gave 7, 0 and 2 failures on successive runs. Claude-Session: https://claude.ai/code/session_01GCdUQC4k8tf93iHnZHSTVS --- .Rbuildignore | 1 + tests/testthat/helper-liftover.R | 3 ++ tests/testthat/helper-test_db.R | 43 +++++++++++++++++-- tests/testthat/test-bigset-character-chrom.R | 2 + tests/testthat/test-bigset-fast-load.R | 2 + tests/testthat/test-db-format-conversion.R | 2 + tests/testthat/test-gdb-convert-parallel.R | 2 + .../testthat/test-gintervals-2d-meta-sparse.R | 2 + .../test-gtrack-create-meta-indexed.R | 2 + .../testthat/test-index-cache-invalidation.R | 2 + tests/testthat/test-indexed-integration.R | 2 + tests/testthat/test-track-indexed-direct.R | 2 + 12 files changed, 62 insertions(+), 3 deletions(-) diff --git a/.Rbuildignore b/.Rbuildignore index 46bb94927..e5e647a4f 100644 --- a/.Rbuildignore +++ b/.Rbuildignore @@ -35,3 +35,4 @@ ^tools$ ^tests/testthat/fixtures/tiny-hub-groot/\.db\.cache$ ^tests/testthat/fixtures/tiny-hub-groot/\.ro_attributes$ +^\.worktrees$ diff --git a/tests/testthat/helper-liftover.R b/tests/testthat/helper-liftover.R index 71664e5b9..471c12d01 100644 --- a/tests/testthat/helper-liftover.R +++ b/tests/testthat/helper-liftover.R @@ -108,6 +108,9 @@ setup_db <- function(chrom_defs, return_db = FALSE) { { unlink(db, recursive = TRUE) unlink(fastas) + # Do not leave GROOT pointing at the directory we just removed: + # the next file in this parallel worker would inherit it. + ensure_valid_groot() }, testthat::teardown_env() ) diff --git a/tests/testthat/helper-test_db.R b/tests/testthat/helper-test_db.R index ada71bba4..122ad951f 100644 --- a/tests/testthat/helper-test_db.R +++ b/tests/testthat/helper-test_db.R @@ -99,13 +99,50 @@ local_db_state <- function(env = parent.frame()) { #' This approach provides complete isolation between parallel test processes #' while minimizing disk space and setup time. #' -#' @return Path to the isolated test database -create_isolated_test_db <- function() { - source_db <- if (getOption("gmulticontig.indexed_format", FALSE)) { +#' Path to the shared, read-only test database +shared_test_db_path <- function() { + if (getOption("gmulticontig.indexed_format", FALSE)) { "/net/mraid20/ifs/wisdom/tanay_lab/tgdata/db/tgdb/misha_test_db_indexed/" } else { "/net/mraid20/export/tgdata/db/tgdb/misha_test_db/" } +} + +#' Guarantee this test file leaves a usable GROOT behind +#' +#' Files that build their own temporary database and re-root into it leave +#' .misha$GROOT dangling as soon as that directory is removed (withr deletes +#' it at the end of the test or the file). Under TESTTHAT_PARALLEL the next +#' file in the same worker process inherits the dangling root and fails with +#' something unrelated to its own subject matter - "Database directory does +#' not exist", "Chromosome chr1 does not exist ... Known chromosomes: chrA", +#' or "Cannot delete track from read-only database" (a deleted directory is +#' not writable, so it reads as read-only). +#' +#' Call this once at the top of any file that re-roots. It is idempotent and +#' does nothing when the file leaves a valid root behind. +ensure_valid_groot <- function() { + groot <- if (exists("GROOT", envir = .misha, inherits = FALSE)) { + get("GROOT", envir = .misha) + } else { + NULL + } + if (is.null(groot) || !dir.exists(groot)) { + src <- shared_test_db_path() + if (dir.exists(src)) { + suppressMessages(gdb.init(src)) + } + } + invisible(NULL) +} + +restore_groot_on_exit <- function(envir = parent.frame()) { + withr::defer(ensure_valid_groot(), envir = envir) +} + +#' @return Path to the isolated test database +create_isolated_test_db <- function() { + source_db <- shared_test_db_path() # Fail loudly instead of leaving a half-built DB behind. A silent failure # here (a full tempdir() is the usual cause) surfaces much later as a diff --git a/tests/testthat/test-bigset-character-chrom.R b/tests/testthat/test-bigset-character-chrom.R index b94f967f8..c1178d1df 100644 --- a/tests/testthat/test-bigset-character-chrom.R +++ b/tests/testthat/test-bigset-character-chrom.R @@ -4,6 +4,8 @@ # files store $chrom as factor while the persisted .meta zeroline kept the # original character class. +restore_groot_on_exit() + test_that("bigset save then load works when input has character chrom", { skip_if_not_installed("withr") withr::local_options(list(gmulticontig.indexed_format = FALSE)) diff --git a/tests/testthat/test-bigset-fast-load.R b/tests/testthat/test-bigset-fast-load.R index d34210945..6522ecdef 100644 --- a/tests/testthat/test-bigset-fast-load.R +++ b/tests/testthat/test-bigset-fast-load.R @@ -1,5 +1,7 @@ # Tests for fast-path loading of indexed bigsets +restore_groot_on_exit() + test_that(".gintervals.is_indexed_bigset detects indexed bigsets correctly", { skip_if_not_installed("withr") withr::local_options(list(gmulticontig.indexed_format = FALSE)) diff --git a/tests/testthat/test-db-format-conversion.R b/tests/testthat/test-db-format-conversion.R index 524b7e912..fb64848bf 100644 --- a/tests/testthat/test-db-format-conversion.R +++ b/tests/testthat/test-db-format-conversion.R @@ -1,3 +1,5 @@ +restore_groot_on_exit() + load_test_db() test_that("gdb.convert_to_indexed converts per-chromosome database to indexed format", { local_db_state() diff --git a/tests/testthat/test-gdb-convert-parallel.R b/tests/testthat/test-gdb-convert-parallel.R index 40e45de8c..64644b92a 100644 --- a/tests/testthat/test-gdb-convert-parallel.R +++ b/tests/testthat/test-gdb-convert-parallel.R @@ -1,3 +1,5 @@ +restore_groot_on_exit() + load_test_db() # Tests for parallel gdb.convert_to_indexed (threads argument). Each test diff --git a/tests/testthat/test-gintervals-2d-meta-sparse.R b/tests/testthat/test-gintervals-2d-meta-sparse.R index b11112f78..78119f6da 100644 --- a/tests/testthat/test-gintervals-2d-meta-sparse.R +++ b/tests/testthat/test-gintervals-2d-meta-sparse.R @@ -4,6 +4,8 @@ # load + query path here with a moderate-sized N (1000 contigs) and only a # handful of populated chrom-pairs. +restore_groot_on_exit() + build_many_contig_db <- function(n_contigs, defer_envir) { skip_if_not_installed("withr") tmp_root <- withr::local_tempdir(.local_envir = defer_envir) diff --git a/tests/testthat/test-gtrack-create-meta-indexed.R b/tests/testthat/test-gtrack-create-meta-indexed.R index e3a2cd6ed..b4c0788fc 100644 --- a/tests/testthat/test-gtrack-create-meta-indexed.R +++ b/tests/testthat/test-gtrack-create-meta-indexed.R @@ -13,6 +13,8 @@ # 2. Meta creation completes for an indexed track where the overwhelming # majority of chromids have no data (the large-contig case). +restore_groot_on_exit() + setup_db <- function(num_chroms, chrom_size = 1e6, indexed = FALSE) { tmp_root <- withr::local_tempdir(.local_envir = parent.frame()) chrom_sizes <- data.frame( diff --git a/tests/testthat/test-index-cache-invalidation.R b/tests/testthat/test-index-cache-invalidation.R index e5223a907..4bf3e739d 100644 --- a/tests/testthat/test-index-cache-invalidation.R +++ b/tests/testthat/test-index-cache-invalidation.R @@ -16,6 +16,8 @@ # These tests exercise the rm-then-recreate cycle and verify the next # read succeeds. +restore_groot_on_exit() + # Build a tiny per-chromosome DB then convert it to indexed format. build_indexed_test_db <- function() { test_db <- tempfile("misha_idxcache_") diff --git a/tests/testthat/test-indexed-integration.R b/tests/testthat/test-indexed-integration.R index 95190da6a..a6fb03ab9 100644 --- a/tests/testthat/test-indexed-integration.R +++ b/tests/testthat/test-indexed-integration.R @@ -1,3 +1,5 @@ +restore_groot_on_exit() + load_test_db() # Integration tests for indexed genome format diff --git a/tests/testthat/test-track-indexed-direct.R b/tests/testthat/test-track-indexed-direct.R index b4633988e..1d31bf8d3 100644 --- a/tests/testthat/test-track-indexed-direct.R +++ b/tests/testthat/test-track-indexed-direct.R @@ -4,6 +4,8 @@ # Build a small indexed DB from a 5-chromosome FASTA and create the # named track via gtrack.create. Returns the path to the track dir. +restore_groot_on_exit() + .make_indexed_db_with_track <- function(track_name, bin_size = 4) { test_fasta <- tempfile(fileext = ".fasta") cat(">chr1\nACTGACTGACTGACTGACTGACTGACTGACTG\n",