-
Notifications
You must be signed in to change notification settings - Fork 123
Add a bit more test coverage for datacube ext #1143
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
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1143 +/- ##
==========================================
+ Coverage 91.25% 91.99% +0.74%
==========================================
Files 49 49
Lines 6642 6611 -31
Branches 981 978 -3
==========================================
+ Hits 6061 6082 +21
+ Misses 408 356 -52
Partials 173 173
☔ View full report in Codecov by Sentry. |
Ok so patch coverage is low, but a significant improvement 🤷 seems to me like it would be ok to override that check, but I could probably also coverage up high enough with another hour or so of work. Just let me know what makes more sense to you @gadomski |
Nah, let's ignore the patch ❌, it seems like too much work to test out all the extension 🍝. |
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.
Minor points of confusion, but LGTM
@gadomski if this one merges it is maybe a little concerning because it was failing coverage. It is fine in this case as mentioned above, but just wanted to flag this. |
|
nice! Ok I just hadn't noticed it was explicitly optional |
Related Issue(s):
Description:
There is already a fair bit of documentation in there, but I improved the test coverage a bit. This is a very good example of an extension class that could/should be substantially simplified in the 2.0 release
PR Checklist:
pre-commit
hooks pass locallyscripts/test
)