-
Notifications
You must be signed in to change notification settings - Fork 28
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
Fix test suite without optional dependencies #182
Fix test suite without optional dependencies #182
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #182 +/- ##
==========================================
+ Coverage 85.93% 86.02% +0.08%
==========================================
Files 79 81 +2
Lines 10365 10387 +22
Branches 2253 2253
==========================================
+ Hits 8907 8935 +28
+ Misses 936 931 -5
+ Partials 522 521 -1
☔ View full report in Codecov by Sentry. |
8a3f8d3
to
b3325e8
Compare
b3325e8
to
bc6d9ad
Compare
bc6d9ad
to
c55eb7e
Compare
@ericpre I can look at this but just to make sure that I understand are there any changes to the emd readers or are they just split? |
Yes, there are just split for the following reasons:
|
rsciio/tests/test_de5.py
Outdated
@@ -25,6 +25,7 @@ | |||
import pytest | |||
|
|||
hs = pytest.importorskip("hyperspy.api", reason="hyperspy not installed") | |||
pytest.importorskip("h5py", reason="hyperspy not installed") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pytest.importorskip("h5py", reason="hyperspy not installed") | |
pytest.importorskip("h5py", reason="h5py not installed") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, I simplified it by removing the reason
argument as it is not necessary.
@ericpre This all seems good, but as for this not being caught it might be good to add a CI which only tests the minimal install for RosettaSciIO? |
My first comment on not being caught was misleading it wasn't initially, when we merged the PR make some of these optional, but It is now being caught on CI: see the last commit on the main branch: https://github.com/hyperspy/rosettasciio/actions/workflows/tests.yml?query=branch%3Amain |
Okay sounds good then! LGTM You can merge it if you would like |
I am not sure why we didn't caught it earlier but I suspect that that installing hyperspy dev will pull these dependencies...
Progress of the PR
jit_if_numba
intvips
reader,upcoming_changes
folder (seeupcoming_changes/README.rst
),docs/readthedocs.org:rosettasciio
build of this PR (link in github checks)