JP-3552: use tmp_path instead of tmpdir, enable no:legacypath as a tox factor #8327

emolter · 2024-03-01T22:17:47Z

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

added entry in CHANGES.rst within the relevant release section
updated or added relevant tests
updated relevant documentation
added relevant milestone
added relevant label(s)
ran regression tests, post a link to the Jenkins job below.
How to run regression tests on a PR
Make sure the JIRA ticket is resolved properly

codecov · 2024-03-04T14:25:42Z

Codecov Report

Attention: Patch coverage is 55.55556% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 53.21%. Comparing base (4cc0ac1) to head (06b0731).
Report is 27 commits behind head on master.

Files	Patch %	Lines
jwst/scripts/okify_regtests.py	0.00%	3 Missing ⚠️
jwst/regtest/conftest.py	75.00%	1 Missing ⚠️

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

Flag	Coverage Δ
nightly	`?`

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

jdavies-st

Looks great! 👏

Just some minor comments (and confusion) below.

jdavies-st · 2024-03-13T15:40:21Z

jwst/background/tests/test_background.py

-    return os.path.join(data_path, filename)
+@pytest.fixture(scope="module")
+def data_path():
+    return pathlib.Path(__file__).parents[0] / "data"


I think Path().parents[0] is the equivalent to Path().parent?

jdavies-st · 2024-03-13T15:52:24Z

jwst/regtest/test_nirspec_fs_spec2.py

@@ -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):


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. =)

good point, yes, there are a lot of these instances. Hopefully I'll get all of them

jdavies-st · 2024-03-13T15:54:27Z

jwst/regtest/test_nirspec_ifu_spec2.py

@@ -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):


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. ;-)

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.

jdavies-st · 2024-03-13T15:54:39Z

jwst/regtest/test_nirspec_ifu_spec2.py

@@ -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):


see comment above

emolter · 2024-03-13T17:12:42Z

New regtest run started here.

edit: Regression tests are failing for the two test_nirspec_ifu_spec2 tests where there seemed to be an implicit scope mismatch before. Of course it Works On My Machine^TM so I started this run that only runs tests in that file.

…d and rtdata together

… sameness is what is being tested

emolter · 2024-03-14T20:47:56Z

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.

jdavies-st

Looks great! Very nice cleanup.

And now you really know how all this regtest infrastructure works. Congrats! 😅

I wouldn't worry about the coverage check. It was jus reporting coverage on the unit tests plus one regtest, so incomplete.

hbushouse · 2024-03-15T12:34:40Z

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.

github-actions bot added assign_wcs associations background cube_build extract_1d jump ramp_fitting skymatch stpipe NIRSPEC MIRI testing exp_to_source documentation installation regression_testing master_background labels Mar 1, 2024

emolter force-pushed the JP-3552 branch from 7195491 to 9fcff43 Compare March 6, 2024 22:16

github-actions bot added outlier_detection resample straylight wfs_combine NIRCAM image2 pipeline mrs_imatch residual_fringe labels Mar 6, 2024

emolter changed the title ~~JP-3552: updated pytest to use tmp_path instead of tmpdir~~ JP-3552: use tmp_path instead of tmpdir, enable no:legacypath as a tox factor Mar 7, 2024

emolter marked this pull request as ready for review March 7, 2024 14:58

emolter requested a review from a team as a code owner March 7, 2024 14:58

jdavies-st approved these changes Mar 13, 2024

View reviewed changes

emolter force-pushed the JP-3552 branch from 6e319bd to 0dcedf4 Compare March 13, 2024 16:44

emolter added 16 commits March 14, 2024 15:52

JP-3552: updated pytest to use tmp_path instead of tmpdir

cf22f64

added changelog entry

782e2ca

pinning asdf oldest dependency

ec78164

using most recent version of ci_watson

717faed

replaced _jail with function_jail

41c260d

revert change to ci-watson dependency

c6ffe44

missed one instance of _jail

9d3291d

replace function_jail with tmp_cwd

ff3cd85

changed os.path.join to /, intermediate commit with known failures

4a8806c

replaced jail with tmp_cwd_module, attempted fix Path validation errors

0384d83

fixing regtest failure in test_infrastructure

2c06ff6

forgot a style check

b6e5c1a

fixes based on @jdavies-st comments, incl remove all passes of tmp_cw…

0949102

…d and rtdata together

revert duplicate rtdata, tmp_cwd in test_infrastructure because their…

39f50e7

… sameness is what is being tested

attempted solve regtest failures in nirspec_ifu_spec2

42f5ed9

made run_photom and run_extract1d function scoped

06b0731

emolter force-pushed the JP-3552 branch from d61999c to 06b0731 Compare March 14, 2024 19:52

emolter requested review from hbushouse and jdavies-st March 14, 2024 20:48

jdavies-st approved these changes Mar 15, 2024

View reviewed changes

hbushouse approved these changes Mar 15, 2024

View reviewed changes

hbushouse merged commit d787bea into spacetelescope:master Mar 15, 2024
27 of 29 checks passed

stscijgbot-jp mentioned this pull request Mar 15, 2024

Use tmp_path instead of tmpdir pytest fixture #8311

Closed

emolter deleted the JP-3552 branch March 15, 2024 13:25

braingram mentioned this pull request Mar 25, 2024

fix fixture for closedfile regtest #8387

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JP-3552: use tmp_path instead of tmpdir, enable no:legacypath as a tox factor #8327

JP-3552: use tmp_path instead of tmpdir, enable no:legacypath as a tox factor #8327

emolter commented Mar 1, 2024 •

edited by hbushouse

Loading

codecov bot commented Mar 4, 2024 •

edited

Loading

jdavies-st left a comment

jdavies-st Mar 13, 2024

jdavies-st Mar 13, 2024

emolter Mar 13, 2024

jdavies-st Mar 13, 2024

emolter Mar 13, 2024

jdavies-st Mar 13, 2024

emolter Mar 13, 2024

emolter commented Mar 13, 2024 •

edited

Loading

emolter commented Mar 14, 2024

jdavies-st left a comment

hbushouse commented Mar 15, 2024

JP-3552: use tmp_path instead of tmpdir, enable no:legacypath as a tox factor #8327

JP-3552: use tmp_path instead of tmpdir, enable no:legacypath as a tox factor #8327

Conversation

emolter commented Mar 1, 2024 • edited by hbushouse Loading

codecov bot commented Mar 4, 2024 • edited Loading

Codecov Report

jdavies-st left a comment

Choose a reason for hiding this comment

jdavies-st Mar 13, 2024

Choose a reason for hiding this comment

jdavies-st Mar 13, 2024

Choose a reason for hiding this comment

emolter Mar 13, 2024

Choose a reason for hiding this comment

jdavies-st Mar 13, 2024

Choose a reason for hiding this comment

emolter Mar 13, 2024

Choose a reason for hiding this comment

jdavies-st Mar 13, 2024

Choose a reason for hiding this comment

emolter Mar 13, 2024

Choose a reason for hiding this comment

emolter commented Mar 13, 2024 • edited Loading

emolter commented Mar 14, 2024

jdavies-st left a comment

Choose a reason for hiding this comment

hbushouse commented Mar 15, 2024

emolter commented Mar 1, 2024 •

edited by hbushouse

Loading

codecov bot commented Mar 4, 2024 •

edited

Loading

emolter commented Mar 13, 2024 •

edited

Loading