JP-3552: use tmp_path instead of tmpdir, enable no:legacypath as a tox factor #8327
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8327 +/- ##
===========================================
- Coverage 75.15% 53.21% -21.95%
===========================================
Files 470 389 -81
Lines 38604 38463 -141
===========================================
- Hits 29014 20468 -8546
- Misses 9590 17995 +8405
☔ View full report in Codecov by Sentry. |
emolter
changed the title
| return os.path.join(data_path, filename) | ||
| @pytest.fixture(scope="module") | ||
| def data_path(): | ||
| return pathlib.Path(__file__).parents[0] / "data" |
There was a problem hiding this comment.
I think Path().parents[0] is the equivalent to Path().parent?
| @@ -35,7 +35,7 @@ | |||
|
|
|||
|
|
|||
| @pytest.fixture(scope="module", params=file_roots) # ids=ids) | |||
| def run_pipeline(jail, rtdata_module, request): | |||
| def run_pipeline(tmp_cwd_module, rtdata_module, request): | |||
There was a problem hiding this comment.
For all these cases where the rtdata_module fixture is used, tmp_cwd_module is not needed, as its already included within rtdata_module. Not a bug, but not necessary.
Same with tests that use the rtdata fixture - no need to use tmp_cwd, as it's already loaded as a fixture when rtdata gets loaded. It's fine to use if you need its return value, but otherwise, it's redundant.
This is not a problem introduced by this PR, as it seems there were already a bunch of tests that used _jail, rtdata and jail, rtdata_module together already.
But it might be best to remove this pattern so that it is not copied further. =)
There was a problem hiding this comment.
good point, yes, there are a lot of these instances. Hopefully I'll get all of them
| @@ -50,7 +50,7 @@ def test_spec2(run_spec2, fitsdiff_default_kwargs, suffix): | |||
|
|
|||
|
|
|||
| @pytest.fixture() | |||
| def run_photom(jail, rtdata): | |||
| def run_photom(tmp_cwd_module, rtdata): | |||
There was a problem hiding this comment.
Not sure what is going on here. one module-scoped fixture (tmp_cwd_module) and one function-scoped fixture ('rtdata') are being used simultaneously. Would be good to sort out what is going on. I can't run regtests over here. ;-)
There was a problem hiding this comment.
looks to me like both the run_photom and run_extract1d fixtures should be module-scoped to permit multiple tests of their output without rerunning. I passed just rtdata_module into these and all the tests in the file pass locally.
| @@ -76,7 +76,7 @@ def test_photom(run_photom, fitsdiff_default_kwargs): | |||
|
|
|||
|
|
|||
| @pytest.fixture() | |||
| def run_extract1d(jail, rtdata): | |||
| def run_extract1d(tmp_cwd_module, rtdata): | |||
…d and rtdata together
… sameness is what is being tested
|
Finally, regression tests for just nirspec_ifu_spec2 passed here. Between this and the previous regtest run, hopefully everything is working now, but I started this new full run just in case. |
|
Latest full regtest run has failures that are all unrelated to this PR (they're due to a separate update to datamodels). So this looks good. I'm approving and merging. Nice work everyone - that's a wrap. |
Resolves JP-3552
Closes #8311
This PR replaces all instances tmpdir and tmpdir_factory pytest fixtures with tmp_path and tmp_path_factory fixtures, as recommended by pytest (see here), so that these are returning standard pathlib.Path instances instead of legacy py.path.local instances.
Jenkins run here
Checklist for maintainers
CHANGES.rstwithin the relevant release sectionHow to run regression tests on a PR