Fixing flaky tests #70

hannahbaumann · 2026-01-19T13:41:28Z

Fixes #43 and #67

This PR:

Closes netcdfs both in the code and in the test
Adds pooch caching back in
Updates the test; the new simulation_skipped dataset is from CDK2, which has two chains. Since the multichain fix is not in yet, this test currently has large RMSDs
Removes flaky argument in tests
Uses the skipped test dataset in more tests for not having to use the bigger one everywhere.

…e runners for the matrix

codecov · 2026-01-19T15:36:21Z

Codecov Report

❌ Patch coverage is 97.43590% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 87.05%. Comparing base (5260e3b) to head (2a1d130).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
src/openfe_analysis/rmsd.py	97.14%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #70      +/-   ##
==========================================
- Coverage   88.16%   87.05%   -1.11%     
==========================================
  Files           7        7              
  Lines         338      340       +2     
==========================================
- Hits          298      296       -2     
- Misses         40       44       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

.github/workflows/ci.yaml

hannahbaumann · 2026-01-28T09:26:19Z

src/openfe_analysis/tests/test_reader.py

        assert ts.time == times[inx]
-        assert np.all(u.atoms.positions > 0)
+        # Positions are not all zero since PBC is not removed
+        assert np.any(u.atoms.positions != 0)


@IAlibay : This is what I mentioned yesterday. The positions in this nc file are not always >0.

Ok thanks, that's interesting..

When I add this u.trajectory.add_transformations(wrap(u.atoms)) it passes the >0 check.

Ok, I think if wrap behaves well then it's fine. We might need to check this later when we do viz thing, but either way it's not like a regression is being added in this code, so we should be good.

hannahbaumann · 2026-01-29T09:25:46Z

pre-commit.ci autofix

for more information, see https://pre-commit.ci

atravitz · 2026-01-29T21:05:09Z

.github/workflows/ci.yaml

+              base_url=ZENODO_DOI,
+              registry=ZENODO_FILES,


glad you're doing this! it's what I'm moving us toward on the openfe side.

atravitz · 2026-01-29T22:22:36Z

@hannahbaumann you'll want to build pooch from main (like we are here in openfe) if you're seeing issues with fetching data.

atravitz · 2026-01-29T21:11:23Z

src/openfe_analysis/tests/test_transformations.py

+    yield u
+    u.trajectory.close()


should we be doing the same here in openfe?

No, xtc files, and trajectories in MDAnalysis overall, do not need this - close should get called on garbage collection and xtc files don't have concurrency issues.

See above - close gets called by __del__ for trajectories, it shouldn't be needed unless there's weird things happening.

src/openfe_analysis/tests/conftest.py

IAlibay

Couple of things, still looking into that positions thing.

.github/workflows/ci.yaml

IAlibay · 2026-02-02T14:07:19Z

src/openfe_analysis/tests/conftest.py

-POOCH_CACHE = pooch.os_cache("openfe_analysis")
+ZENODO_DOI = "doi:10.5281/zenodo.18378051"
+
+ZENODO_FILES = {


We should probably have an "HAS_INTERNET" check on most of our tests.
Maybe todo in a separate issue?

IAlibay · 2026-02-02T14:23:01Z

src/openfe_analysis/tests/test_reader.py



 def test_universe_creation(simulation_nc, hybrid_system_pdb):
-    with pytest.warns(UserWarning, match="This is an older NetCDF file that"):


Do we still have a test somewhere to warn for the old netcdf file types?

If not, can we mock one?

IAlibay · 2026-02-02T14:27:08Z

src/openfe_analysis/tests/test_reader.py

-    assert_allclose(u.dimensions, [82.191055, 82.191055, 82.191055, 90.0, 90.0, 90.0])
+    assert_allclose(u.dimensions, [78.141495, 78.141495, 78.141495, 60.0, 60.0, 90.0])

+    u.trajectory.close()


Are these actually necessary now? __del__ should call self.close (it's a reader thing), so they should get garbage collected when they go out of scope, if they don't then we might still be encountering an issue with closing the files.

I'm not seeing failures without this, I had put it since I wasn't sure if things could be unstable when lots of test run in parallel, but we could remove it for now and if we see problems later again, investigate further?

Since it's only in tests, I would vote for removing and seeing how we fare - it's a good canary.

IAlibay · 2026-02-02T14:28:36Z

src/openfe_analysis/tests/test_transformations.py

+    yield u
+    u.trajectory.close()


See above - close gets called by __del__ for trajectories, it shouldn't be needed unless there's weird things happening.

Co-authored-by: Irfan Alibay <[email protected]>

IAlibay · 2026-02-02T15:36:40Z

.github/workflows/ci.yaml

+          key: pooch-${{ matrix.os }}-v2
+
+      - name: "Download Zenodo data"
+        shell: python


Ah sorry @hannahbaumann - I was wrong, it looks like this might use some other python (not the one from micromamba)!

hannahbaumann added 8 commits January 16, 2026 15:57

Move some tests to use skipped smaller data

b528ca2

Test out zenodo dealings

a477bc1

Try to improbe speed

ad84082

Try removing locking

8ba8087

Run downloads before the testing to have a single download for all th…

ead7951

…e runners for the matrix

add import pooch

f898a35

Test out more

c675a5c

Ensure datasets get closed

88e456d

hannahbaumann added 9 commits January 19, 2026 16:40

Move to per test download again

73a8e4d

Remove commented out lines

43aaca2

Test out adding an extra slash

c165525

Switch to all version doi

5f17770

Download url directly

c28286e

Small fix

197b6ba

Change url

b45390a

Add missing s

1d70936

Switch to api url

20084c3

IAlibay mentioned this pull request Jan 21, 2026

Datasets not being closed properly #71

Open

Update tests for new results

a9a8780

jameseastwood assigned hannahbaumann Jan 26, 2026

hannahbaumann added 8 commits January 26, 2026 17:30

Update conftest

c34c97c

Update to v2

0161673

Update tests

9b6ca69

Update rmsd test, currently large rmsd till rmsd fix comes in

1d5c849

Make last test pass

f4e88e2

Switch to zenodo fetch

bd0c8ee

remove lines

ba4c912

Update tests with large errors multichain failure

157c02f

hannahbaumann commented Jan 28, 2026

View reviewed changes

.github/workflows/ci.yaml Outdated Show resolved Hide resolved

hannahbaumann and others added 5 commits January 28, 2026 09:36

Apply suggestion from @hannahbaumann

98ea023

Reuse zenodo specification

c5b2d70

reorder install

54576ab

Small fix

3aa52a5

Remove flaky retries

ff6991a

hannahbaumann commented Jan 28, 2026

View reviewed changes

hannahbaumann changed the title ~~[WIP ] Closing netcdfs properly~~ Fixing flaky tests Jan 28, 2026

hannahbaumann requested review from IAlibay and atravitz January 28, 2026 09:29

Small fix

7a30f69

This was referenced Jan 28, 2026

Fix multi chain RMSD analysis #64

Open

improve file clean up #67

Open

hannahbaumann linked an issue Jan 28, 2026 that may be closed by this pull request

improve file clean up #67

Open

hannahbaumann mentioned this pull request Jan 28, 2026

Tests are flaky - unclosed NetCDF datasets? #43

Open

hannahbaumann linked an issue Jan 28, 2026 that may be closed by this pull request

Tests are flaky - unclosed NetCDF datasets? #43

Open

jameseastwood assigned atravitz and IAlibay and unassigned hannahbaumann Jan 28, 2026

[pre-commit.ci] auto fixes from pre-commit.com hooks

1b2c488

for more information, see https://pre-commit.ci

atravitz reviewed Jan 29, 2026

View reviewed changes

Apply review suggestions

2a1d130

IAlibay requested changes Feb 2, 2026

View reviewed changes

Update .github/workflows/ci.yaml

209620a

Co-authored-by: Irfan Alibay <[email protected]>

IAlibay reviewed Feb 2, 2026

View reviewed changes



		def test_universe_creation(simulation_nc, hybrid_system_pdb):
		with pytest.warns(UserWarning, match="This is an older NetCDF file that"):

Fixing flaky tests #70

Are you sure you want to change the base?

Fixing flaky tests #70

Uh oh!

Conversation

hannahbaumann commented Jan 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jan 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hannahbaumann commented Jan 29, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atravitz commented Jan 29, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

IAlibay left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hannahbaumann commented Jan 19, 2026 •

edited

Loading

codecov bot commented Jan 19, 2026 •

edited

Loading