search

spacetelescope / stdatamodels

https://stdatamodels.readthedocs.io
Other
5 stars 25 forks source link

Check datamodels against schema #324

Closed tapastro closed 2 months ago

tapastro commented 2 months ago

Partially addresses #323

This PR adds a test to check schemas against instance of jwst datamodels. Might want to add additional test to address second bullet of #323.

Checklist

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 66.18%. Comparing base (84279cf) to head (455b03d). Report is 5 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #324 +/- ## ======================================= Coverage 66.18% 66.18% ======================================= Files 100 100 Lines 5397 5397 ======================================= Hits 3572 3572 Misses 1825 1825 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

braingram commented 2 months ago

Thanks for the PR. I am a little confused about our CI at the moment. Let me try to explain.

The asdf pytest plugin is configured to run with this repo: https://github.com/spacetelescope/stdatamodels/blob/4e37d607f86059d995b95f506d255052b90aed01/pyproject.toml#L139 and the plugin runs check_schema.

When I test this locally (by making a schema invalid) I see the expected failure (using modifications off main). So I think we are should already be testing the schemas in the way proposed in this PR. However, since the tests on https://github.com/spacetelescope/stdatamodels/pull/320 didn't fail for an invalid schema I think our github CI might be misconfigured (perhaps the tests are running from a different path).

I'll take a look but I think once we sort out the CI we should automatically have the schemas tests as is done in this PR.

braingram commented 2 months ago

I opened a test PR with the pastasoss schema changed back to the type: float64 and the CI fails for that schema (as it should): https://github.com/spacetelescope/stdatamodels/pull/325 I'm not sure why we didn't see the failure before (or perhaps we didn't give it enough time to fail?).

Overall I think that means that we don't need a separate check_schema for the schemas (since pytest-asdf is already handling that for us). I'll update the issue.

braingram commented 2 months ago

I should also say that I wouldn't be opposed to disabling pytest-asdf and adding our own schema tests (similar to what's in this PR). It would allow the eventual removal of pytest-asdf (although it is used in many other places). I don't think pytest-asdf is providing either enough functionality to be useful and given issues like what came up here (where it should have caught an error but didn't) doesn't give me much confidence.

tapastro commented 2 months ago

Closing in favor of #327