Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Isolate jwst/romancal pytest configuration #297

Merged
merged 3 commits into from
Oct 10, 2024

Conversation

braingram
Copy link
Collaborator

@braingram braingram commented Oct 2, 2024

Currently downstream testing of jwst and romancal end up using the pyproject.toml in this repository for determining pytest configuration.
https://github.com/spacetelescope/stcal/actions/runs/11142379456/job/30965215790#step:10:107
This ends up with pytest using options in these downstream tests that differ from the options used by the downstream packages in their tests (when run in their own CI). This in turn results in lots of false failures in our downstream testing.

This PR changes how the jwst and romancal downstream tests to use the pytest options defined in the respective repositories.

Tasks

  • update or add relevant tests
  • update relevant docstrings and / or docs/ page
  • Does this PR change any API used downstream? (if not, label with no-changelog-entry-needed)
    • write news fragment(s) in changes/: echo "changed something" > changes/<PR#>.<changetype>.rst (see below for change types)
    • run regression tests with this branch installed ("git+https://github.com/<fork>/stcal@<branch>")
news fragment change types...
  • changes/<PR#>.apichange.rst: change to public API
  • changes/<PR#>.bugfix.rst: fixes an issue
  • changes/<PR#>.general.rst: infrastructure or miscellaneous change

@braingram braingram changed the title attempt to isolate jwst/romancal pytest configuration Isolate jwst/romancal pytest configuration Oct 2, 2024
Copy link

codecov bot commented Oct 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.21%. Comparing base (a83e20b) to head (60bd3b8).
Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #297      +/-   ##
==========================================
+ Coverage   81.80%   86.21%   +4.40%     
==========================================
  Files          46       47       +1     
  Lines        8810     8812       +2     
==========================================
+ Hits         7207     7597     +390     
+ Misses       1603     1215     -388     

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

@braingram braingram force-pushed the downstream_ci branch 2 times, most recently from 97f5a2d to f96d709 Compare October 2, 2024 18:54
@braingram braingram marked this pull request as ready for review October 2, 2024 19:44
@braingram braingram requested a review from a team as a code owner October 2, 2024 19:44
Copy link
Collaborator

@kmacdonald-stsci kmacdonald-stsci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a changes fragment for the change log, but otherwise fine.

@braingram
Copy link
Collaborator Author

Needs a changes fragment for the change log, but otherwise fine.

Thanks! I pushed on in 24b0a2b I gave it the 'general' category. I originally left one off since this is all internal/testing changes. If it's helpful to have these in the changelog we could add a 'testing' catagory for these types of changes.

@jhunkeler
Copy link
Contributor

jhunkeler commented Oct 10, 2024

Hey, the test-numpy{...} environments don't map back to any [testenv].deps. Unless you planned to get rid of these the following should make it work:

diff --git a/tox.ini b/tox.ini
index 14937c2..8819ecf 100644
--- a/tox.ini
+++ b/tox.ini
@@ -2,7 +2,7 @@
 envlist =
     check-{style,build,types}
     test{,-warnings,-cov}-xdist
-    test-numpy{120,121,122}
+    test-numpy{123,124,125,126}
     test-{jwst,romancal}{,-xdist}{,-cov}
     build-{docs,dist}
 
@@ -69,6 +69,10 @@ deps =
     devdeps: asdf @ git+https://github.com/asdf-format/asdf.git
     devdeps: drizzle @ git+https://github.com/spacetelescope/drizzle.git
     devdeps: gwcs @ git+https://github.com/spacetelescope/gwcs.git
+    numpy123: numpy<1.24.0,>=1.23.0
+    numpy124: numpy<1.25.0,>=1.24.0
+    numpy125: numpy<1.26.0,>=1.25.0
+    numpy126: numpy<2.0.0,>=1.26.0
 use_develop = true
 pass_env =
     CI

EDIT: Tox silently ignored 1.20 through 1.22. Packages in the environment must be forcing it to update to 2.x.

@braingram
Copy link
Collaborator Author

Thanks for finding that. I removed those from the envlist in 60bd3b8 The oldestdeps job should check the lower numpy pin, the "normal" jobs the latest release and the devdeps the dev version so I think we have numpy well covered by those.

@jhunkeler
Copy link
Contributor

No problem! I don't see anything else that jumps out at me.

@braingram braingram merged commit d64107a into spacetelescope:main Oct 10, 2024
26 checks passed
@braingram braingram deleted the downstream_ci branch October 10, 2024 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants