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

(fix): move zarr import inside try-catch in test helpers #1343

Merged
merged 6 commits into from
Jan 26, 2024

Conversation

ilan-gold
Copy link
Contributor

@ilan-gold ilan-gold commented Jan 26, 2024

Is there some sort of way to test this? Open to suggestions. ruff does not support custom rules, I think?

@ilan-gold ilan-gold changed the title (fix): move zarr import inside try-catch (fix): move zarr import inside try-catch in test helpers Jan 26, 2024
@ilan-gold ilan-gold self-assigned this Jan 26, 2024
Comment on lines 783 to 787
class AccessTrackingStore:
def __init__(self) -> None:
raise ImportError(
"zarr must be imported to create an `AccessTrackingStore` instance."
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a huge fan of this, but I think the other option is to create AccessTrackingStore based on some dummy base class, which will error out on construction with a less helpful error. And if we want to raise an error, we're sort of back to this.

@ivirshup
Copy link
Member

I think an anndata-mindeps test job would be the way to test this, and then testing all the functionality used by scanpy-mindeps uses under that job. I don't think a static check would be enough here, since a something could always be imported dynamically

Copy link

codecov bot commented Jan 26, 2024

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (e07477d) 85.71% compared to head (8f1d056) 83.49%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1343      +/-   ##
==========================================
- Coverage   85.71%   83.49%   -2.23%     
==========================================
  Files          34       34              
  Lines        5455     5460       +5     
==========================================
- Hits         4676     4559     -117     
- Misses        779      901     +122     
Flag Coverage Δ
gpu-tests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
anndata/tests/helpers.py 86.99% <80.00%> (-9.20%) ⬇️

... and 6 files with indirect coverage changes

@ilan-gold ilan-gold requested a review from ivirshup January 26, 2024 13:36
@ivirshup ivirshup added this to the 0.10.6 milestone Jan 26, 2024
anndata/tests/helpers.py Outdated Show resolved Hide resolved
@ilan-gold ilan-gold merged commit 299ca97 into main Jan 26, 2024
13 checks passed
@ilan-gold ilan-gold deleted the fix_zarr_import_test_helpers branch January 26, 2024 14:39
meeseeksmachine pushed a commit to meeseeksmachine/anndata that referenced this pull request Jan 26, 2024
ivirshup pushed a commit that referenced this pull request Jan 26, 2024
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