reduce non-determinism in tests #1347
Conversation
drammock
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1347 +/- ##
=======================================
Coverage 97.42% 97.42%
=======================================
Files 40 40
Lines 8883 8893 +10
=======================================
+ Hits 8654 8664 +10
Misses 229 229 ☔ View full report in Codecov by Sentry. |
| shutil.copytree(return_bids_test_dir, bids_root) | ||
|
|
||
| # without providing all the entities, ambiguous when trying | ||
| # to use fpath | ||
| # without providing all the entities, ambiguous when trying to use fpath |
There was a problem hiding this comment.
unrelated change, just a reflow
| def test_rm(return_bids_test_dir, capsys, tmp_path): | ||
| """Test BIDSPath's rm method to remove files.""" | ||
| # for some reason, mne's logger can't be captured by caplog.... | ||
| bids_root = str(tmp_path_factory.mktemp("test_rm") / "mnebids_utils_test_bids_ds") | ||
| bids_root = tmp_path / "mnebids_utils_test_bids_ds" |
There was a problem hiding this comment.
I don't think it makes sense to use tmp_path_factory here, as it is session-scoped and we don't need this extra dir to persist beyond the life of this one test.
| # should then return None. | ||
| # Move the fine-calibration file. BIDSPath.meg_calibration_fpath should then be None | ||
| bids_path_ = _bids_path.copy().update(subject="01", root=bids_root) | ||
| Path(bids_path_.meg_calibration_fpath).unlink() | ||
| src = Path(bids_path_.meg_calibration_fpath) | ||
| src.rename(tmp_path / src.name) | ||
| assert bids_path_.meg_calibration_fpath is None | ||
| # restore the file | ||
| (tmp_path / src.name).rename(src) | ||
| assert bids_path_.meg_calibration_fpath is not None |
There was a problem hiding this comment.
this essentially just changes a Path(...).unlink() to a Path(...).rename(...), then moves the file back once we're done testing that its absence yields the desired result.
This is important because the file in question is in a session-scoped dataset fixture, so simply deleting the file means that other tests that expect it to be there will fail if they are executed after this test
| # return None. | ||
| # Move the crosstalk file. BIDSPath.meg_crosstalk_fpath should then be None. | ||
| bids_path = _bids_path.copy().update(subject="01", root=bids_root) | ||
| Path(bids_path.meg_crosstalk_fpath).unlink() | ||
| src = Path(bids_path.meg_crosstalk_fpath) | ||
| src.rename(tmp_path / src.name) | ||
| assert bids_path.meg_crosstalk_fpath is None | ||
| # restore the file | ||
| (tmp_path / src.name).rename(src) | ||
| assert bids_path.meg_crosstalk_fpath is not None |
There was a problem hiding this comment.
same change as for the fine-calibration file above: move,test,move-back instead of unlink.
There was a problem hiding this comment.
thanks a lot for digging into this @drammock. Please ping me once you are done working on it.
I'm done. Only failing CI is In the long run, we should consider a fixture to automatically check the state of the test dir before & after each test, to ensure no files are changed. Not sure if this could be built into |
|
Thanks! |
PR Description
closes #1346
Merge checklist
Maintainer, please confirm the following before merging.
If applicable: