search

spacetelescope / romancal

Python library to process science observations from the Nancy Grace Roman Space Telescope
https://roman-pipeline.readthedocs.io/en/latest/
Other
32 stars 28 forks source link

Use tmp_path instead of tmpdir pytest fixture #1115

Open jdavies-st opened 8 months ago

jdavies-st commented 8 months ago

Replace all instances tmpdir and tmpdir_factory pytest fixtures with tmp_path and tmp_path_factory fixtures, respectively.

Then in pyproject.toml:

[tool.pytest.ini_options]
addopts = [
    "-p no:legacypath",
]

which will prevent it from being used in the future. This is will make the code more robust for use of pathlib.Path instances in the runtime code.

nden commented 8 months ago

Thanks! Fixed by #1108

braingram commented 8 months ago

I'm going to reopen this to track the addition of no:legacypath to pyproject.toml. This is currently held up by the pytest-asdf plugin (which has been fixed in main but not yet released).

braingram commented 8 months ago

This is also blocked by ddtrace (which uses the legacy path as part of it's plugin): https://github.com/spacetelescope/romancal/actions/runs/8071483455/job/22051208643?pr=1117#step:10:215

braingram commented 8 months ago

This is also blocked by ci_watson using tmpdir https://github.com/spacetelescope/ci_watson/blob/2fc02e8312310753c88ff0a3c4bf16cb545205e8/ci_watson/plugin.py#L70 and by uses of tmp_factory in the romancal regtests (see this run that disabled ddtrace for the regtests and shows 26 errors): https://plwishmaster.stsci.edu:8081/blue/organizations/jenkins/RT%2FRoman-Developers-Pull-Requests/detail/Roman-Developers-Pull-Requests/613/pipeline/220

braingram commented 8 months ago

A PR is open to update the tmpdir usage in ci_watson: https://github.com/spacetelescope/ci_watson/pull/64

braingram commented 8 months ago

https://github.com/spacetelescope/romancal/pull/1117 enabled this option for unit tests.

enabling it for regression tests is blocked by: https://github.com/DataDog/dd-trace-py/issues/8533

I vote for leaving this open to track the issue, once ddtrace is updated we can enable the regression tests and close the issue.

emolter commented 7 months ago

I happened to run across this line that seems to still point to _jail; looks to me like this should be changed before merging.

braingram commented 7 months ago

Thanks @emolter for the find. I took a look and that fixture/class was unused. It's now been removed in: https://github.com/spacetelescope/romancal/pull/1145