jwst
jwst copied to clipboard
spacetelescope
Use tmp_path instead of tmpdir pytest fixture
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.
Comment by Ned Molter on JIRA:
for doctests, following example from Astropy: pytest discussion question from pllim
Comment by Ned Molter on JIRA:
The use of "-p no:legacypath" currently causes failures when running tests with both the GitHub Action and Jenkins that are due to the dependency on ci_watson. ci_watson was updated 4 days ago to remove instances of "tmpdir" and use "-p no:legacypath" but a release has not yet been published. Brett Graham mentioned that ci_watson is little maintained and could possibly be replaced in the future. He suggested two possible paths forward:
- apply similar fixes to jwst as was in this romancal PR (this would duplicate fixtures in these two packages but allow each to make their own modifications)
- update
ci_watsonto make_jailpublic and use it in both packages
Does anyone have opinions on which of these is the preferred option?
Comment by Tyler Pauly on JIRA:
I don't know much about ci_watson, but its description sounds as though its for Jenkins testing - is it also used for GitHub Actions? Our future testing plans include a migration from Jenkins to GH Actions - will we still be using ci_watson when that happens? If so, it may make sense to get an update into it - if not, probably best not to worry about that side and get the non-ci_watson fix included. Not sure if the proposed replacement package that James brought up will be used or not.
All this to say - it may make sense to ask Joe Hunkeler or Zach Burnett, as the choice is probably weighted most heavily by the future of our test architecture.
Comment by Joe Hunkeler on JIRA:
Repositories that deal with input data and "truth" files for testing use ci_watson to generate Artifactory "spec" files. When a test failure occurs pytest's basetemp contains the (potential truth) files. These paths are recorded in a JSON config. Jenkins uses an Artifactory plugin to process the configs, but honestly, it's equivalent to running this in an Action:
find $PATH_TO_PYTEST_BASETEMP -type -name 'result_*.json' | xargs -I '{}' jf rt u --spec '{}'
I would drop the use of ci_watson to provide _jail and just write your own tmp_cwd based on pytest tmp_path fixture. Just use ci_watson for the time being for its Artifactory interface tools. They are really separate things and don't depend on one another.
Making a tmp_cwd pytest fixture to replace _jail is easy. I've done it for astroquery, and another package I maintain:
https://github.com/mpi-astronomy/snowblind/blob/b58f5b30287737c84ce0b5624af81c6f65f4a1c1/tests/conftest.py#L7-L15
Comment by Ned Molter on JIRA:
Ok, thanks both of you! The tmp_cwd fixture is basically the same as the function_jail that Roman implemented, but I'm going to use tmp_cwd because I think the name is more descriptive. And thanks for the clarification about how those two uses of ci_watson are independent, I agree it is beyond the scope of this ticket (if even desired at present) to replace the artifactory interface tools
There's also a module-scoped pytest fixture called jail in this package
https://github.com/spacetelescope/jwst/blob/911b5c67d126a9939011509c5aed09df8e236420/jwst/conftest.py#L54-L69
which is used extensively in the regression tests, and it would be worthwhile changing that one to use tmp_path_factory as well. And if you're improving the names to something descriptive, I'd recommend changing its name to something descriptive like tmp_cwd_module_scoped or something more catchy if like.