Split long regtests into reusable fixtures and smaller named tests #1426
Conversation
braingram
changed the title
| def test_output_is_image_model(output_model): | ||
| # DMS280 FIXME comment says 280 | ||
| # DMS414 FIXME log message said 414 | ||
| # FIXME was not an assert |
There was a problem hiding this comment.
The current code:
romancal/romancal/regtest/test_wfi_16resultants.py
Lines 98 to 102 in e741d19
Mentions DMS280 in the comment but DMS414 in the log message and contains no assert. What is the expected DMS requirement here?
There was a problem hiding this comment.
414 is right. 413 in the ~docstring and 280 are wrong. This is also correct (414) in https://github.com/spacetelescope/romancal/blob/e741d1963e163d8fbbeff714cb6a2acea662ae8a/romancal/tests/dms_requirement_tests.json .
There was a problem hiding this comment.
Thanks! I updated the comments here: 457073d
|
|
||
| @pytest.mark.soctests | ||
| def test_has_a_wcs(output_model): | ||
| # DMS129 TODO not listed |
There was a problem hiding this comment.
DMS129 isn't listed in the requirements. Is it satisfied?
There was a problem hiding this comment.
Yes, we should mark this as done. I can't figure out why it's not picked up by John's page, though.
There was a problem hiding this comment.
Added DMS129 in 4c0ad1d
| ) | ||
| def test_steps_ran(output_model, step_name): | ||
| # DMS86 | ||
| # also DMS129 for assign_wcs |
There was a problem hiding this comment.
There was a problem hiding this comment.
DMS129 should be marked done (i.e., we should add it to the test_requirements_dms.json file)
There was a problem hiding this comment.
Added DMS129 in 4c0ad1d
| @pytest.mark.soctests | ||
| @pytest.mark.parametrize("meta_attribute", ("detector", "optical_element")) | ||
| def test_instrument_meta(output_model, meta_attribute): | ||
| # DMS-136 PSF tests TODO not listed |
There was a problem hiding this comment.
DMS136 isn't listed in the requirements. Is it satisfied?
There was a problem hiding this comment.
Yes, we should go ahead and mark it. I don't understand why it's not tracked on John's "missing requirements" page.
There was a problem hiding this comment.
Thanks! Added DMS136 in 229b164
|
|
||
| @pytest.mark.soctests | ||
| def test_wcs_has_bounding_box(output_model): | ||
| # DMS89 WCS tests TODO not listed |
There was a problem hiding this comment.
DMS89 is not listed in the requirements. Is it satisfied?
There was a problem hiding this comment.
This is likewise a requirement that should be added to the requirements.json list; we've delivered this requirement and this was the test that did that.
There was a problem hiding this comment.
Added DMS89 in 03ce0ad
| def test_resample_ran(output_model): | ||
| # DMS373 test output is resampled to a skycell | ||
| # FIXME this doesn't test if the output is a skyceell | ||
| assert output_model.meta.cal_step.resample == "COMPLETE" |
There was a problem hiding this comment.
The previous code:
romancal/romancal/regtest/test_mos_pipeline.py
Lines 151 to 154 in e741d19
mentions that the requirement is that the output is resampled to a skycell. The test doesn't appear to test that (and was not previously an assert).
There was a problem hiding this comment.
I agree that the comment is misleading. Let's leave this for now. Perhaps the test should be updated to look for https://github.com/spacetelescope/rad/blob/main/src/rad/resources/schemas/mosaic_basic-1.0.0.yaml#L133 , but I'm not sure that we actually populate that right now. I'll spin that into a separate issue.
|
I'm going to open this for review to try to get some help on the questions about the DMS requirements. There are a number of inconsistencies and missing requirements (ones mentioned in the code but not in the requirement file, for the ones I investigated they also did not previously have a decorator). |
braingram
force-pushed
the
refactor_regtests
branch
from
0e28096 to
308b4fc
Compare
Closes #1325
This PR refactors some large regtests:
passfailfunctions and their use (the test results provide information about if the test passed or failed)There are a few items to resolve for this PR:
dms_requirement_tests.jsonneed parametrization values (and are they supported) for parameterized tests that correspond to dms requirements? (for exampleromancal.regtest.test_mos_pipeline.test_steps_rannow tests DMS400 but technically only using theskymatchparametrization) EDIT: @zacharyburnett confirmed the non-parametrized one is the one to use.dms_requirement_tests.json(and did not previously have a decorator). These log messages aren't in a format that the metrics collector understands. If these were added to verify a requirement those requirements are not being verified. I suspect we want to convert the log messages into entries intodms_requirement_tests.jsonand will add comments for those dms requirements.Regression tests all pass: https://github.com/spacetelescope/RegressionTests/actions/runs/11071508240
Tasks
24Q4_B15(use the latest build if not sure)no-changelog-entry-needed)changes/:echo "changed something" > changes/<PR#>.<changetype>.rst(see below for change types)docs/pageokify_regteststo update the truth filesnews fragment change types...
changes/<PR#>.general.rst: infrastructure or miscellaneous changechanges/<PR#>.docs.rstchanges/<PR#>.stpipe.rstchanges/<PR#>.associations.rstchanges/<PR#>.scripts.rstchanges/<PR#>.mosaic_pipeline.rstchanges/<PR#>.patch_match.rststeps
changes/<PR#>.dq_init.rstchanges/<PR#>.saturation.rstchanges/<PR#>.refpix.rstchanges/<PR#>.linearity.rstchanges/<PR#>.dark_current.rstchanges/<PR#>.jump_detection.rstchanges/<PR#>.ramp_fitting.rstchanges/<PR#>.assign_wcs.rstchanges/<PR#>.flatfield.rstchanges/<PR#>.photom.rstchanges/<PR#>.flux.rstchanges/<PR#>.source_detection.rstchanges/<PR#>.tweakreg.rstchanges/<PR#>.skymatch.rstchanges/<PR#>.outlier_detection.rstchanges/<PR#>.resample.rstchanges/<PR#>.source_catalog.rst