Give each test its own scratch directory and each TestScheduler test its own fixtures - #1028
Open
calvinp0 wants to merge 8 commits into
Open
Give each test its own scratch directory and each TestScheduler test its own fixtures#1028calvinp0 wants to merge 8 commits into
calvinp0 wants to merge 8 commits into
Conversation
This was referenced Aug 27, 2026
ARC_TESTING_PATH holds read-only fixture inputs. Passing a subdirectory of it as a project_directory made every test class in these modules write into one shared, fixed path, and tearing that path down with shutil.rmtree at the end of the class. Under pytest-xdist a single TestCase class is split across workers, so every worker runs tearDownClass. One worker therefore deletes the shared directory while another is still writing input files into it, which surfaces as FileExistsError / FileNotFoundError on an unrelated test. A crashed run also leaves the directory behind inside the repository. Each class now allocates its scratch with tempfile.mkdtemp() in setUpClass and releases it with addClassCleanup. mkdtemp is unique per setUpClass invocation, so concurrent workers never share a path and nothing is ever written under arc/testing. Read-only fixture reads (the PBS time-limit err.txt in adapter_test) still come from ARC_TESTING_PATH and are unchanged.
Same change as for the ESS job adapters: the TS search adapter tests wrote their project directories into ARC_TESTING_PATH and removed them from tearDownClass or from ad-hoc per-test cleanup helpers. Scratch now comes from tempfile.mkdtemp() and is released with addClassCleanup / addCleanup. The KinBot helper that removed a single test's subdirectory and then attempted to rmdir the shared parent existed only to make the shared parent safe under xdist; with a per-class temporary root the whole tree is released at once and the helper is dropped. gcn_test.py also drops the `from arc.job.adapters.ts.gcn_ts import GCNAdapter` line, which duplicated the `import arc.job.adapters.ts.gcn_ts as ts_gcn` already present; kinbot_test.py imports unittest.mock as mock rather than `from unittest import mock` alongside `import unittest`.
These modules built their project directories under ARC_TESTING_PATH and removed them in tearDownClass, which is not safe when xdist splits the class across workers. Both UMA test classes now allocate their own mkdtemp root, so the env-independent and model-dependent classes can no longer reach into each other's tree either. arc/job/adapters/pyscf_test.py is left as main has it: PR #970 already moved that class onto tempfile.mkdtemp(), so the isolation is in place and reapplying it in a second style would only churn the file.
Four tests treated a fixture directory as writable scratch: - common_test.test_globalize_paths wrote restart_paths_globalized.yml into arc/testing/restart/4_globalized_paths, which needed a self-healing cleanup helper called from both setUpClass and tearDownClass to keep the artifact from leaking into the repo. The fixture is now copied into a temporary directory and globalized there, and the two assertions that hard-coded the arc/testing path now assert against the temporary project directory. - common_test.test_safe_copy_file copied its destination file next to the source fixture. - processor_test.test_compare_rates wrote rate plots into arc/testing/process_kinetics. - reaction_test.test_load_ts_xyz_user_guess_from_files passed the fixture directory itself as an ARC project_directory, so a Scheduler run created arc.log, output/ and log_and_restart_archive/ inside arc/testing/reactions/methanoate_hydrolysis. The fixture is now copied to a temporary directory first; the TS guess filenames in input_1.yml and input_2.yml resolve relative to project_directory, so the copy keeps them working. - arkane_test.TestArkaneAdapter released its mkdtemp from tearDownClass rather than addClassCleanup, and wrote generate_arkane_input output into arc/testing.
The TestNMD and TestTSChecks classes built their job project directories under ARC's shared Projects folder and deleted them in tearDownClass. pytest-xdist may split a single TestCase class across workers, so each worker runs setUpClass and tearDownClass independently and one worker's teardown removes a directory another worker is still using. Allocate the scratch with tempfile.mkdtemp() in setUpClass and release it with addClassCleanup instead.
…irectories TestARC instantiated ARC without a project directory, so every instance created a directory named after its project under ARC's shared Projects folder, several of which were never deleted. TestPlotter additionally wrote its multi-species logs, its BDE report and its IRC animation into the read-only arc/testing fixture tree, where one test asserted the absence of files another test had created. Give each of them a tempfile.mkdtemp() scratch directory released through addCleanup or addClassCleanup, and drop the tearDownClass removals of shared paths. The gcn_tst cleanup entry in species_test is dropped as well; nothing creates that directory any more.
The restart tests derived their project directories from a per-xdist-worker project name, which does not help when pytest-xdist splits the class across workers. Allocate each project directory with tempfile.mkdtemp() and release it with addCleanup.
…ther TestScheduler built its schedulers, species and jobs once in setUpClass and shared them across every test in the class, while eleven of those tests mutate what they are handed: they replace scheduler.output, inject species into species_dict and job_dict, flip job_types['rotors'], overwrite job_status and output paths, and append to unique_species_labels. All of them also shared a single fixed project directory that tearDownClass deleted. Under pytest-xdist the class is split across workers, each running its own class-level setup and teardown, so one worker's tearDownClass removed the project directory another worker was still writing into, and a test that had implicitly relied on an earlier test dirtying the shared scheduler saw a clean one instead. Roughly half of the runs failed, with a different set of tests each time. Build the fixtures per test in setUp instead, and give every test its own project directory removed via addCleanup, which also lets tearDownClass go. Constructing the fixtures costs about 60 ms, so the serial run of the file grows from ~5.0 s to ~6.7 s and the -n 4 run is unchanged. test_initialize_output_dict opened by asserting that the shared output dict already contained information, which only held because an earlier test had put it there; it failed when run on its own. It now writes that information itself before asserting.
calvinp0
force-pushed
the
fix_test_isolation_sweep
branch
from
August 27, 2026 21:24
a4f2fe7 to
9611d7b
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1028 +/- ##
==========================================
+ Coverage 64.76% 64.80% +0.03%
==========================================
Files 119 119
Lines 40039 40039
Branches 10350 10350
==========================================
+ Hits 25932 25948 +16
+ Misses 11129 11119 -10
+ Partials 2978 2972 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
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.
Supersedes #1008 and #1009, which are closed in favour of this branch. Eight commits, no file touched by more than one of them.
Tests wrote their scratch into
arc/testing, the tree that holds read-only fixture inputs, andTestSchedulershared one scheduler and one project directory across its tests. Under-n auto --dist=workstealthat makes the outcome depend on which tests happen to land on a worker together, so the same commit passes and fails across runs.What it changes
Writable scratch comes from
tempfile.mkdtemp(), released withaddCleanup/addClassCleanup, so no two tests can contend for a path and nothing is left behind when a run dies.arc/testingis read from and not written to.TestSchedulerbuilds its own scheduler and project directory per test.Across the tree, passing
ARC_TESTING_PATHas aproject_directorygoes from 15 files to 2, andrmtreeof a subdirectory of it from 8 to 3.Measured
Order dependence is only visible if the order is varied, so the suite was run under
pytest-random-orderwith--random-order-bucket=global, which shuffles across files, at eight fixed seeds. The floor is 5: thearc/job/adapters/torch_ani_test.pytests, which fail on an unsetTANI_PYTHONand are unrelated to ordering.mainSix of the eight land on the floor exactly. The failures that disappear at every seed are
arc/checks/ts_test.py::test_check_rxn_e0and::test_compute_rxn_e0,arc/job/adapter_test.py::test_determine_job_status,arc/scheduler_test.py::test_initialize_output_dict, and thextb,obabel,xtbgsmandprocessoradapter tests.Serial runs are identical either side,
5 failed, 2924 passed, 43 skipped, so nothing here trades parallel stability for a serial regression.The two seeds that do not reach the floor
Seeds 202 and 707 leave one failure each, both in
test_GaussianAdapter:arc/job/adapters/common_test.pyandarc/job/adapters/gaussian_test.pyboth useARC_TESTING_PATH/test_GaussianAdapteras a project directory and bothrmtreeit, so one module's teardown deletes the directory the other is writing into. Neither file is touched here. Running only those two files against this branch's base under-n 4 --dist workstealfails 3 times out of 3, so the race is a property ofmainrather than of this change: removing the other collisions stops masking it. Those two files, andcommon_test.py'stest_MolproAdapterdirectory, are what the counts above do not cover.How the two branches were combined
#1008's seven commits were rebased onto current
mainand #1009's single commit cherry-picked on top.mainhad moved a long way, and some of #1008's work had landed in the meantime:arc/job/adapters/pyscf_test.pywas already converted totempfile.mkdtemp(), somain's version is kept verbatim and the file drops out of this branch rather than being converted a second time in a different style.Where
mainhad added tests to a file #1008 also changed, both are kept.arc/checks/nmd_test.pyandarc/checks/ts_test.pykeep the sulfur and chlorine fixtures and the mode-convention assertions;arc/job/adapters/ase_test.pykeeps the relaxed rotor scan tests, and itsos.makedirs(..., exist_ok=True)guard against the worker race is removed by the isolation rather than left alongside it;arc/statmech/arkane_test.pykeeps both new tests and retains itsARC_TESTING_PATHimport, which it uses correctly to read fixture inputs.arc/checks/nmd_test.pyalso carries one change that is not a conflict resolution: a test added onmainused a fixedARC_PATH/Projects/tmp_nmd_unsupported_ess_projectdirectory, which is shared across workers in the same way, and it now usestempfile.mkdtemp().