-
Notifications
You must be signed in to change notification settings - Fork 1
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
Progress on catalog version reporting #295
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #295 +/- ##
==========================================
- Coverage 98.49% 98.20% -0.29%
==========================================
Files 11 11
Lines 996 1058 +62
==========================================
+ Hits 981 1039 +58
- Misses 15 19 +4 ☔ View full report in Codecov by Sentry. |
I added more tests, but codecov still thinks I'm not testing L60-63 of |
I'll take a proper look at this first thing tomorrow morning. |
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.
I suspect that codecov might be getting upset because of the way the mocks are defined?
eg. test_data:
@mock.patch("access_nri_intake.data.utils.get_catalog_fp")
-77 def test_available_versions_no_catalog(mock_get_catalog_fp):
-78 mock_get_catalog_fp.return_value = "/this/is/not/real.yaml"
-79 with pytest.raises(FileNotFoundError):
-80 available_versions()
+ @mock.patch("access_nri_intake.data.utils.get_catalog_fp")
+ def test_available_versions_no_catalog(mock_get_catalog_fp):
+ mock_get_catalog_fp.return_value = "/this/is/not/real.yaml"
+ with pytest.raises(FileNotFoundError):
+ available_versions()
Intellisense is complaining to me that the mock_get_catalog_fp
fixture is unused - maybe that's what's throwing codecov off the trail?
Otherwise, a few minor nitpicks (I think I'm a bit jumpy after letting the overflow bug into main) but otherwise looks good.
Whoops, accidentally requested a review instead of giving one... |
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.
Looks to me like codecov is talking nonsense, so I think this is ready to merge.
Progress on #294 .
This PR updates the existing
available_versions
function to:pathlib.Path
instead of theos
module;catalog.yaml
when reporting available versions;pretty
printout return.Tests have also been updated as required.