chore: de-flake the parallel test suite + clear the CRAN hidden-files NOTE - #152
Open
aviezerl wants to merge 2 commits into
Open
chore: de-flake the parallel test suite + clear the CRAN hidden-files NOTE#152aviezerl wants to merge 2 commits into
aviezerl wants to merge 2 commits into
Conversation
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
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Test-harness only — no changes to package code. All of this came out of diagnosing #151, where the local suite gave 7, then 0, then 2 failures on effectively identical code.
The bug
Every one of those failures was the same thing wearing a different mask:
.misha$GROOTleft pointing at a directory that had been deleted. UnderTESTTHAT_PARALLELtestthat runs several files per worker process, so file N can strand file N+1. The symptoms looked unrelated to each other and unrelated to their own test files:Interval test.fixedbin does not existChromosome chr1 does not exist ... Known chromosomes: chrACannot delete track from read-only database(a removed directory isn't writable, sofile.accessreports it read-only)Two halves:
create_isolated_test_db()'s teardown calledrm("GROOT")when the previous root had already been removed — which is exactly what happens when the preceding file in the worker was itself isolated. The worker was left with no database at all.withrdelete the directory while still rooted there.27 files re-root without restoring, but only the 10 that delete the directory mid-run can strand a later file.
ensure_valid_groot()/restore_groot_on_exit()inhelper-test_db.Rhandle both halves; forhelper-liftover.Rthe restore goes into its existingteardown_env()defer, which covers every file that uses it.Also
create_isolated_test_db()ignored the exit status of itscp -r/ln -scalls, so a fulltempdir()produced a half-built DB that failed much later somewhere else. It now errors at the point of failure and namestempdir(). This earned its keep immediately: it fired zero times during the FAIL 7 run, which ruled it out as the cause in one grep..Rbuildignorethe two hidden files undertests/testthat/fixtures/tiny-hub-groot. Their only consumer isskip_on_cran()-gated. Verified against a real tarball: no hidden files at all, fixture content intact..Rbuildignore ^\.worktrees$. Local git worktrees live inside the repo, soR CMD buildwas sweeping all 15 into the tarball — 3.1 GB instead of 1.8 MB, which made localR CMD build/checkunusable. No effect on CI, which builds from a fresh clone.CONTRIBUTING.mddocuments theTMPDIRrequirement for parallel runs, and the tell that failures moving between files across runs mean disk exhaustion rather than flaky tests.Verification
alutil::tst(parallel = TRUE)three consecutive times:FAIL 0 | WARN 92 | SKIP 32 | PASS 19271, identical each run. The pass count matches the pre-change green run, so nothing is being skipped.Not claimed: that this makes the suite provably deterministic. Three clean runs is evidence, not proof, and the remaining 17 re-rooting files are fine only because they don't delete their DB mid-run.